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

hborla · 2023-06-10T08:46:11Z

This PR is a follow-up to #64968 that replaces the request evaluator stack approach to macro expansion suppression in macro arguments with a source-location-based approach using ASTScope.

Background

SE-0389 states that code in a macro argument does not have visibility into macro-generated names for macros at the same scope or enclosing scopes, because the semantics of the program would depend on the ordering of those macro expansions. Arbitrary macro names also make name lookup extremely prone to request cycles, because any lookup into that scope will need to expand macros that can produce arbitrary names. For these reasons, macro resolution cannot be dependent on macro expansion, so macro-generated names are excluded from name lookup when lookup happens within a macro argument.

The old request-evaluator-stack approach

Previously, answering the question of "are we performing name lookup from within a macro argument?" used a heuristic that checked whether there is an active ResolveMacroRequest in the current request-evaluator stack. This heuristic was not totally correct. For example, if type checking a macro argument was the first bit of type checker work that invoked conformance checking for a type, name lookups performed by the conformance checker would exclude macro-generated names even though the conforming type is not inside of a macro argument. If the conformance checker happens to be invoked before type checking the macro argument, macro-generated names would be included. Because macro-generated names in a conforming type can fulfill protocol requirements, this makes the type checking semantics dependent on the order of requests, which is Not Good.

The new `ASTScope` approach

Instead, this change uses source locations to answer the question of "are we performing name lookup from within a macro argument?". Source locations are now plumbed through all of the name lookup APIs. When constructing the various name lookup requests, the source location is used to determine whether lookup is being done from within a macro argument. We use ASTScopes to find the innermost scope enclosing that source location, and walk up the scope tree to look for either a freestanding macro expansion scope or an attached macro scope. If we find one of those scopes, that source location is contained within a macro argument, and the appropriate flag is added to the name lookup options to exclude macro-generated names from the name lookup results.

Arbitrary names at global scope

As part of this, I needed to ban freestanding and peer macro expansions at global scope that can generate 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. If anything the macro depends on performs 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.

Resolves rdar://109214279.

hborla · 2023-06-10T23:20:39Z

lib/AST/ASTScopeLookup.cpp

+    // macro expansion buffers for freestanding macros are children of
+    // MacroExpansionDeclScope, and child scopes of freestanding macros
+    // are otherwise inside the macro argument.
+    if (scope->getClassName() == "ASTSourceFileScope")


Is there a better way to check the ASTScope kind?

No, and it looks like you're not the first to lament that this is the only way to check:

// FIXME: Proper isa/dyn_cast support would be better than a string // comparison here. while (parent->getClassName() == "GenericParamScope") parent = parent->getLookupParent().get();

hborla · 2023-06-10T23:46:14Z

@swift-ci please smoke test

lib/AST/NameLookupRequests.cpp

fbartho · 2023-06-11T04:04:06Z

Re: Using freestanding & arbitrary peer names now being banned at the global level.

Are you sure that’s what you want to do? I feel like plenty of @Model style macros for other engines might need to add arbitrary objects to the global scope.

I’m sure I don’t understand the code at this level though, so please disregard if I don’t make much sense.

hborla · 2023-06-11T04:07:44Z

Are you sure that’s what you want to do? I feel like plenty of @model style macros for other engines might need to add arbitrary objects to the global scope.

This change does not affect the @Model macro, the @Observable macro, or other similar macros that add members to types. And the ban is specifically on arbitrary names; freestanding and peer macros can still add named, prefixed, suffixed, and overloaded names to the global scope.

hborla · 2023-06-11T06:40:08Z

@swift-ci please smoke test

DougGregor

This looks fantastic. The restriction on arbitrary names at global scope is going to be a bit of a shock at first, but I think we can find suitable alternative approaches. I think I'm convincing myself that it's better overall for readability to limit the introduction of globals.

include/swift/AST/ASTScope.h

DougGregor · 2023-06-11T16:00:52Z

include/swift/AST/Decl.h

@@ -3891,7 +3891,7 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
  /// protocols to which the nominal type conforms. Furthermore, the resulting
  /// set of declarations has not been filtered for visibility, nor have
  /// overridden declarations been removed.
-  TinyPtrVector<ValueDecl *> lookupDirect(DeclName name,
+  TinyPtrVector<ValueDecl *> lookupDirect(DeclName name, SourceLoc loc = SourceLoc(),


Should we plan to take away that default argument at some point, once we've managed to update all of the callers?

include/swift/AST/NameLookup.h

DougGregor · 2023-06-11T16:03:59Z

lib/AST/ASTScopeLookup.cpp

+    // macro expansion buffers for freestanding macros are children of
+    // MacroExpansionDeclScope, and child scopes of freestanding macros
+    // are otherwise inside the macro argument.
+    if (scope->getClassName() == "ASTSourceFileScope")


No, and it looks like you're not the first to lament that this is the only way to check:

// FIXME: Proper isa/dyn_cast support would be better than a string // comparison here. while (parent->getClassName() == "GenericParamScope") parent = parent->getLookupParent().get();

lib/AST/NameLookupRequests.cpp

DougGregor · 2023-06-11T16:07:29Z

lib/Sema/PreCheckExpr.cpp

-      TypeChecker::performTypoCorrection(DC, UDRE->getRefKind(), Type(),
-                                         lookupOptions, corrections);
+
+      // FIXME: Don't perform typo correction inside macro arguments, because it


Argh, this is a tricky one. Maybe typo correction should avoid ever synthesizing declarations or expanding macros.

fbartho · 2023-06-11T20:41:46Z

And the ban is specifically on arbitrary names; freestanding and peer macros can still add named, prefixed, suffixed, and overloaded names to the global scope.

Ah! This reassures me/addresses my fear. I thought this was a flat ban on any new names (introduced by macros) at global scope.

Thanks for clarifying!

hborla · 2023-06-12T03:14:32Z

@swift-ci please smoke test

…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.

…s inside a macro argument.

…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.

expansions. Evaluator::hasActiveResolveMacroRequest is now unused.

checker while type checking a freestanding macro argument in the same scope.

…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.

… of a macro argument.

…lookup.

…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.

…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.

hborla · 2023-06-12T06:11:19Z

@swift-ci please smoke test

hborla · 2023-06-12T15:34:48Z

@swift-ci please smoke test Windows

hborla · 2023-06-12T16:21:57Z

@swift-ci please smoke test Windows

WowbaggersLiquidLunch · 2023-06-13T00:45:47Z

As part of this, I needed to ban freestanding and peer macro expansions at global scope that can generate arbitrary names.

If I understand it correctly, does this mean something like the #gyb example in SE-0397 is no longer possible?

hborla · 2023-06-13T01:07:53Z

If I understand it correctly, does this mean something like the #gyb example in SE-0397 is no longer possible?

That's correct. However, that example is not producing truly arbitrary names, and it could be expressed if a freestanding macro could use prefixed, e.g. prefixed(Int), prefixed(UInt), etc. We're working on a proposal revision that 1) specifies the restriction implemented in this PR, and 2) allows freestanding macros to use prefixed and suffixed so that we don’t lose all of the expressivity of dynamically generating global names in freestanding declaration macros.

WowbaggersLiquidLunch · 2023-06-13T13:04:05Z

We're working on a proposal revision that 1) specifies the restriction implemented in this PR, and 2) allows freestanding macros to use prefixed and suffixed so that we don’t lose all of the expressivity of dynamically generating global names in freestanding declaration macros.

Ok this seems reasonable to me. At least, prefixed and suffixed should be sufficient for my own use case.

hborla commented Jun 10, 2023

View reviewed changes

hborla marked this pull request as ready for review June 10, 2023 23:46

hborla requested review from ahoppen, bnbarham, rintaro, AnthonyLatsis, slavapestov, xedin, artemcm, tshortli, zoecarver, hyp and egorzhdan as code owners June 10, 2023 23:46

hborla requested review from DougGregor and removed request for hyp, egorzhdan, artemcm, zoecarver, tshortli and AnthonyLatsis June 10, 2023 23:47

hborla commented Jun 10, 2023

View reviewed changes

lib/AST/NameLookupRequests.cpp Show resolved Hide resolved

hborla requested a review from CodaFi as a code owner June 11, 2023 06:39

hborla removed the request for review from CodaFi June 11, 2023 06:41

DougGregor approved these changes Jun 11, 2023

View reviewed changes

hborla added 14 commits June 11, 2023 23:09

[ASTScope] Rename and generalize AttachedPropertyWrapperScope to Cust…

abcdc6c

…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.

[ASTScope] Add attribute scopes for nominal type and extension scopes.

2c0c596

[ASTScope] Add APIs for determining whether a given source location i…

cd79cf9

…s inside a macro argument.

[Macros] Always exclude macro expansions in TypeChecker::lookupMacros.

2943064

[NameLookup] Use source locations to determine whether to exclude macro

61c99cc

expansions. Evaluator::hasActiveResolveMacroRequest is now unused.

[PreCheckExpr] Don't perform typo correction inside macro arguments.

db23cf5

[Macros] Add a test that invokes macro expansion through the conformance

f9f63e3

checker while type checking a freestanding macro argument in the same scope.

[ASTScope] Don't consider freestanding macro expansions as being part…

0b91f63

… of a macro argument.

[CodeCompletion] Plumb source locations through code completion name …

706411d

…lookup.

[Evaluator] Remove hasActiveResolveMacroRequest().

2cc1204

hborla force-pushed the suppress-expansion-in-macro-argument branch from 2ae6396 to 2cc1204 Compare June 12, 2023 06:10

xedin removed their request for review June 12, 2023 16:31

hborla merged commit fc24471 into swiftlang:main Jun 12, 2023

hborla deleted the suppress-expansion-in-macro-argument branch June 12, 2023 18:18

hborla mentioned this pull request Jun 12, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hborla commented Jun 10, 2023 •

edited

Loading

hborla Jun 10, 2023

DougGregor Jun 11, 2023

hborla commented Jun 10, 2023

fbartho commented Jun 11, 2023

hborla commented Jun 11, 2023 •

edited

Loading

hborla commented Jun 11, 2023

DougGregor left a comment •

edited

Loading

DougGregor Jun 11, 2023

DougGregor Jun 11, 2023

DougGregor Jun 11, 2023

fbartho commented Jun 11, 2023 •

edited

Loading

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

WowbaggersLiquidLunch commented Jun 13, 2023

hborla commented Jun 13, 2023

WowbaggersLiquidLunch commented Jun 13, 2023

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

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

Conversation

hborla commented Jun 10, 2023 • edited Loading

Background

The old request-evaluator-stack approach

The new ASTScope approach

Arbitrary names at global scope

hborla Jun 10, 2023

Choose a reason for hiding this comment

DougGregor Jun 11, 2023

Choose a reason for hiding this comment

hborla commented Jun 10, 2023

fbartho commented Jun 11, 2023

hborla commented Jun 11, 2023 • edited Loading

hborla commented Jun 11, 2023

DougGregor left a comment • edited Loading

Choose a reason for hiding this comment

DougGregor Jun 11, 2023

Choose a reason for hiding this comment

DougGregor Jun 11, 2023

Choose a reason for hiding this comment

DougGregor Jun 11, 2023

Choose a reason for hiding this comment

fbartho commented Jun 11, 2023 • edited Loading

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

hborla commented Jun 12, 2023

WowbaggersLiquidLunch commented Jun 13, 2023

hborla commented Jun 13, 2023

WowbaggersLiquidLunch commented Jun 13, 2023

hborla commented Jun 10, 2023 •

edited

Loading

The new `ASTScope` approach

hborla commented Jun 11, 2023 •

edited

Loading

DougGregor left a comment •

edited

Loading

fbartho commented Jun 11, 2023 •

edited

Loading