Skip to content

[5.9][Macros] Use source locations to determine whether to suppress macro expansions. #66578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Jun 13, 2023
Merged

[5.9][Macros] Use source locations to determine whether to suppress macro expansions. #66578

merged 14 commits into from
Jun 13, 2023

Conversation

hborla
Copy link
Member

@hborla hborla commented Jun 12, 2023

  • Explanation: A detailed explanation is in the description of [Macros] Use source locations to determine whether to suppress macro expansions. #66532. The short version is that this change fixes a source of bogus type checker errors that were caused by incorrectly identifying code as being part of a macro argument and excluding macro-generated names from name lookup as a result.
  • Issue: rdar://109214279
  • Risk: There's a moderate risk of new circular reference errors when applying macros, because the old approach was not sound and it would incorrectly suppress macro expansions for code that needs to expand macros. However, code that would produce circular reference errors is actually circular and needs to be changed. The current behavior will depend on the ordering of macro expansions, which is very bad, and this fix will eliminate that.
  • Testing: Added new tests to the suite.
  • Reviewer: @DougGregor
  • Main branch PR: [Macros] Use source locations to determine whether to suppress macro expansions. #66532

hborla added 14 commits June 12, 2023 13:50
@hborla
[ASTScope] Rename and generalize AttachedPropertyWrapperScope to Cust…
804eab8 
…omAttributeScope

so that it can represent the scope for any custom attribute and its arguments.

This commit is NFC, but CustomAttributeScope is now applicable to attached macros.

(cherry picked from commit abcdc6c)
@hborla
[ASTScope] Add attribute scopes for nominal type and extension scopes.
5dbe6ac 
(cherry picked from commit 2c0c596)
@hborla
[ASTScope] Add APIs for determining whether a given source location i…
d226b08 
…s inside

a macro argument.

(cherry picked from commit cd79cf9)
@hborla
[Macros] Always exclude macro expansions in TypeChecker::lookupMacros.
68b8c69 
(cherry picked from commit 2943064)
@hborla
[NameLookup] Plumb source location arguments through all name lookup …
6097df7 
…APIs.

This source location will be used to determine whether to add a name lookup
option to exclude macro expansions when the name lookup request is constructed.
Currently, the source location argument is unused.

(cherry picked from commit cd752cc)
@hborla
[NameLookup] Use source locations to determine whether to exclude macro
dfc4c9a 
expansions.

Evaluator::hasActiveResolveMacroRequest is now unused.
(cherry picked from commit 61c99cc)
@hborla
[PreCheckExpr] Don't perform typo correction inside macro arguments.
8d2da14 
(cherry picked from commit db23cf5)
@hborla
[Macros] Add a test that invokes macro expansion through the conformance
3f9fc44 
checker while type checking a freestanding macro argument in the same scope.

(cherry picked from commit f9f63e3)
@hborla
[Macros] Ban freestanding and peer macro expansions that can produce …
0ad8532 
…arbitrary

names at global scope.

Freestanding and peer macros applied at top-level scope cannot introduce
arbitrary names. Introducing arbitrary names means that any lookup
into this scope must expand the macro. This is a problem, because
resolving the macro can invoke type checking other declarations, e.g.
anything that the macro arguments depend on. If _anything_ the macro
depends on performs name unqualified name lookup, e.g. type resolution,
we'll get circularity errors. It's better to prevent this by banning
these macros at global scope if any of the macro candidates introduce
arbitrary names.

(cherry picked from commit 9824954)
@hborla
[ASTScope] Don't consider freestanding macro expansions as being part…
e411877 
… of a

macro argument.

(cherry picked from commit 0b91f63)
@hborla
[CodeCompletion] Plumb source locations through code completion name …
41b3f35 
…lookup.

(cherry picked from commit 706411d)
@hborla
[Parser] Reset ASTScopes before performing the IDEInspection second p…
732acad 
…ass.

IDE inspection can delay parsing of particular declarations, so expanding
ASTScopes during the first pass will miss those declarations. Clear any
expanded scopes to force re-expansion during the second pass.

(cherry picked from commit 51e1a39)
@hborla
[NameLookup] Move macro-related name lookup operations into the namel…
9a3d531 
…ookup

namespace.

This moves the `isInMacroArgument` predicate and `lookupMacros` into `namelookup`.
ASTScope still encapsulates the scope tree and contains the operation to lookup
the enclosing macro scope, which then invokes a callback to determine whether a
potential macro scope is indeed a macro, because answering this question requires
name lookup.

(cherry picked from commit 393b4ce)
@hborla
[Evaluator] Remove hasActiveResolveMacroRequest().
454bb30 
(cherry picked from commit 2cc1204)
@hborla hborla marked this pull request as ready for review June 12, 2023 22:19
@hborla hborla requested a review from a team as a code owner June 12, 2023 22:19
@hborla
Copy link
Member Author

hborla commented Jun 12, 2023

@swift-ci please test

@hborla hborla merged commit b6318f2 into swiftlang:release/5.9 Jun 13, 2023
@hborla hborla deleted the 5.9-suppress-expansion-in-macro-argument branch June 13, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hborla @DougGregor