Skip to content

[CodeCompletion] Suggest synthesized declarations from macros #66821

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3827,6 +3827,10 @@ bool ValueDecl::shouldHideFromEditor() const {
getBaseIdentifier().str().startswith("$__"))
return true;

// Macro unique names are only intended to be used inside the expanded code.
if (MacroDecl::isUniqueMacroName(getBaseName()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

return true;

return false;
}

Expand Down
49 changes: 29 additions & 20 deletions lib/AST/Module.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,6 @@ class swift::SourceLookupCache {
SourceLookupCache(const SourceFile &SF);
SourceLookupCache(const ModuleDecl &Mod);

/// Throw away as much memory as possible.
void invalidate();

void lookupValue(DeclName Name, NLKind LookupKind,
OptionSet<ModuleLookupFlags> Flags,
SmallVectorImpl<ValueDecl*> &Result);
Expand Down Expand Up @@ -552,6 +549,29 @@ void SourceLookupCache::lookupVisibleDecls(ImportPath::Access AccessPath,
Consumer.foundDecl(vd, DeclVisibilityKind::VisibleAtTopLevel);
}
}

populateAuxiliaryDeclCache();
SmallVector<MissingDecl *, 4> unexpandedDecls;
for (auto &entry : TopLevelAuxiliaryDecls) {
for (auto &decl : entry.second) {
unexpandedDecls.append(entry.second.begin(), entry.second.end());
}
}

// Store macro expanded decls in a 'SmallSetVector' because different
// MissingDecls might be created by a single macro expansion. (e.g. multiple
// 'names' in macro role attributes). Since expansions are cached, it doesn't
// cause duplicated expansions, but different 'unexpandedDecl' may report the
// same 'ValueDecl'.
SmallSetVector<ValueDecl *, 4> macroExpandedDecls;
for (MissingDecl *unexpandedDecl : unexpandedDecls) {
unexpandedDecl->forEachMacroExpandedDecl([&](ValueDecl *vd) {
macroExpandedDecls.insert(vd);
});
}
for (auto *vd : macroExpandedDecls) {
Consumer.foundDecl(vd, DeclVisibilityKind::VisibleAtTopLevel);
}
}

void SourceLookupCache::lookupClassMembers(ImportPath::Access accessPath,
Expand Down Expand Up @@ -608,16 +628,6 @@ void SourceLookupCache::lookupClassMember(ImportPath::Access accessPath,
results.append(iter->second.begin(), iter->second.end());
}

void SourceLookupCache::invalidate() {
TopLevelValues.clear();
ClassMembers.clear();
MemberCachePopulated = false;

// std::move AllVisibleValues into a temporary to destroy its contents.
using SameSizeSmallVector = decltype(AllVisibleValues);
(void)SameSizeSmallVector{std::move(AllVisibleValues)};
}

//===----------------------------------------------------------------------===//
// Module Implementation
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -1067,9 +1077,12 @@ void SourceFile::lookupValue(DeclName name, NLKind lookupKind,
void ModuleDecl::lookupVisibleDecls(ImportPath::Access AccessPath,
VisibleDeclConsumer &Consumer,
NLKind LookupKind) const {
if (isParsedModule(this))
return getSourceLookupCache().lookupVisibleDecls(
AccessPath, Consumer, LookupKind);
if (isParsedModule(this)) {
auto &cache = getSourceLookupCache();
cache.lookupVisibleDecls(AccessPath, Consumer, LookupKind);
assert(Cache.get() == &cache && "cache invalidated during lookup");
return;
}

FORWARD(lookupVisibleDecls, (AccessPath, Consumer, LookupKind));
}
Expand Down Expand Up @@ -3524,7 +3537,6 @@ void ModuleDecl::clearLookupCache() {
return;

// Abandon any current cache. We'll rebuild it on demand.
Cache->invalidate();
Cache.reset();
}

Expand Down Expand Up @@ -3944,9 +3956,6 @@ SynthesizedFileUnit &FileUnit::getOrCreateSynthesizedFile() {
return *thisSynth;
SynthesizedFile = new (getASTContext()) SynthesizedFileUnit(*this);
SynthesizedFileAndKind.setPointer(SynthesizedFile);
// Rebuild the source lookup caches now that we have a synthesized file
// full of declarations to look into.
getParentModule()->clearLookupCache();
}
return *SynthesizedFile;
}
Expand Down
37 changes: 33 additions & 4 deletions lib/AST/NameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ void UsableFilteringDeclConsumer::foundDecl(ValueDecl *D,
if (auto *contextD = DC->getAsDecl())
tmpLoc = contextD->getStartLoc();
}
if (!SM.isBeforeInBuffer(D->getLoc(), Loc))
auto declLoc = DC->getParentModule()->getOriginalLocation(D->getLoc()).second;
if (!SM.isBeforeInBuffer(declLoc, tmpLoc))
return;
}

Expand Down Expand Up @@ -3875,20 +3876,48 @@ void FindLocalVal::visitBraceStmt(BraceStmt *S, bool isTopLevelCode) {
return;
}

// Visit inner statements first before reporting local decls in the current
// scope.
for (auto elem : S->getElements()) {
// If we have a SingleValueStmtExpr, there may be local bindings in the
// wrapped statement.
if (auto *E = elem.dyn_cast<Expr *>()) {
if (auto *SVE = dyn_cast<SingleValueStmtExpr>(E))
visit(SVE->getStmt());
continue;
}
if (auto *S = elem.dyn_cast<Stmt*>())

if (auto *S = elem.dyn_cast<Stmt*>()) {
visit(S);
continue;
}
}
for (auto elem : S->getElements()) {
if (auto *D = elem.dyn_cast<Decl*>()) {

auto visitDecl = [&](Decl *D) {
if (auto *VD = dyn_cast<ValueDecl>(D))
checkValueDecl(VD, DeclVisibilityKind::LocalVariable);
D->visitAuxiliaryDecls([&](Decl *D) {
if (auto *VD = dyn_cast<ValueDecl>(D))
checkValueDecl(VD, DeclVisibilityKind::LocalVariable);
// FIXME: Recursively call `visitDecl` to handle nested macros.
});
};

for (auto elem : S->getElements()) {
if (auto *E = elem.dyn_cast<Expr *>()) {
// 'MacroExpansionExpr' at code-item position may introduce value decls.
// NOTE: the expression must be type checked.
// FIXME: In code-completion local expressions are _not_ type checked.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we get completions from macro expansion declarations if the corresponding MacroExpansionExpr hasn’t been type-checked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't. So we need to typecheck MacroExpansionExpr before here, or here.

if (auto *mee = dyn_cast<MacroExpansionExpr>(E)) {
if (auto *med = mee->getSubstituteDecl()) {
visitDecl(med);
}
}
continue;
}
if (auto *D = elem.dyn_cast<Decl*>()) {
visitDecl(D);
continue;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions lib/SILOptimizer/Differentiation/LinearMapInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ LinearMapInfo::createBranchingTraceDecl(SILBasicBlock *originalBB,
branchingTraceDecl->setAccess(AccessLevel::Internal);
}
file.addTopLevelDecl(branchingTraceDecl);
file.getParentModule()->clearLookupCache();

return branchingTraceDecl;
}
Expand Down
27 changes: 12 additions & 15 deletions lib/Sema/LookupVisibleDecls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,17 +217,24 @@ static void collectVisibleMemberDecls(const DeclContext *CurrDC, LookupState LS,
Type BaseType,
IterableDeclContext *Parent,
SmallVectorImpl<ValueDecl *> &FoundDecls) {
for (auto Member : Parent->getMembers()) {
auto *VD = dyn_cast<ValueDecl>(Member);
auto check = [&](Decl *decl) {
auto *VD = dyn_cast<ValueDecl>(decl);
if (!VD)
continue;
return;
if (!isDeclVisibleInLookupMode(VD, LS, CurrDC))
continue;
return;
if (!evaluateOrDefault(CurrDC->getASTContext().evaluator,
IsDeclApplicableRequest(DeclApplicabilityOwner(CurrDC, BaseType, VD)),
false))
continue;
return;
FoundDecls.push_back(VD);
};

for (auto Member : Parent->getMembers()) {
check(Member);
Member->visitAuxiliaryDecls([&](Decl *d) {
check(d);
});
}
}

Expand Down Expand Up @@ -634,16 +641,6 @@ static void synthesizeMemberDeclsForLookup(NominalTypeDecl *NTD,
false);
}

// Expand peer macros.
for (auto *member : NTD->getMembers()) {
if (!ctx.evaluator.hasActiveRequest(ExpandPeerMacroRequest{member})) {
(void)evaluateOrDefault(
ctx.evaluator,
ExpandPeerMacroRequest{member},
{});
Comment on lines -638 to -643
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary because visitAuxiliaryDecls() calls it lazily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What calls visitAuxiliaryDecls for members in regular type checking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's populateLookupTableEntryFromMacroExpansions called from DirectLookupRequest::evaluate()

}
}

synthesizePropertyWrapperVariables(NTD);
}

Expand Down
96 changes: 96 additions & 0 deletions test/IDE/complete_macros_expanded.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// REQUIRES: swift_swift_parser

// RUN: %empty-directory(%t)
// RUN: mkdir -p %t/plugins

//##-- Prepare the macro plugin.
// RUN: %host-build-swift -swift-version 5 -emit-library -o %t/plugins/%target-library-name(MacroDefinition) -module-name=MacroDefinition %S/../Macros/Inputs/syntax_macro_definitions.swift -g -no-toolchain-stdlib-rpath

// RUN: %target-swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t -plugin-path %t/plugins -parse-as-library

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since arbitrary goes down fairly different paths to named, possibly worth adding tests for it as well?

Copy link
Member Author

@rintaro rintaro Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently arbitrary at top-level is not really supported:
If we add names: arbitrary

  • error: 'declaration' macros are not allowed to introduce arbitrary names at global scope
  • error: 'peer' macros are not allowed to introduce arbitrary names at global scope

And the macro decl becomes isInvalid(), and the expansion is not performed.

If we omit names from the macro decl:

  • error: declaration name '<name>' is not covered by macro '<macro name>'

Introduced values are found and type checked, but since it's error, it doesn't built.

I feel like we should remove

   unexpandedDecls.append(TopLevelArbitraryMacros.begin(),
                          TopLevelArbitraryMacros.end());

From lookupVisibleDecls()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking within a struct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non top level cases, there is not much different code paths between arbitrary and named, but sure.

@freestanding(declaration, names: named(A), named(B), named(foo), named(addOne))
macro defineDeclsWithKnownNames() = #externalMacro(module: "MacroDefinition", type: "DefineDeclsWithKnownNamesMacro")

@attached(peer, names: suffixed(_peer))
macro PeerWithSuffix() = #externalMacro(module: "MacroDefinition", type: "PeerValueWithSuffixNameMacro")

@attached(peer, names: arbitrary)
macro PeerWithSuffixAsArbitrary() = #externalMacro(module: "MacroDefinition", type: "PeerValueWithSuffixNameMacro")

@freestanding(declaration, names: arbitrary)
macro VarValueDecl() = #externalMacro(module: "MacroDefinition", type: "VarValueMacro")


#defineDeclsWithKnownNames

@PeerWithSuffix
func globalFunc() {}

func test() {
#^GLOBAL^#
// GLOBAL-DAG: Decl[Struct]/CurrModule: A[#A#]; name=A
// GLOBAL-DAG: Decl[Struct]/CurrModule: B[#B#]; name=B
// GLOBAL-DAG: Decl[GlobalVar]/CurrModule: foo[#Int#]; name=foo
// GLOBAL-DAG: Decl[GlobalVar]/CurrModule: addOne[#(Int) -> Int#]; name=addOne
// GLOBAL-DAG: Decl[FreeFunction]/CurrModule: globalFunc()[#Void#]; name=globalFunc()
// GLOBAL-DAG: Decl[GlobalVar]/CurrModule: globalFunc_peer[#Int#]; name=globalFunc_peer
}

struct MyStruct {
@PeerWithSuffix
func instanceMethod() {}

@PeerWithSuffix
static func staticMethod() {}

@PeerWithSuffixAsArbitrary
func forArbitrary() {}

#defineDeclsWithKnownNames

#VarValueDecl
}

func testMemberInstance(value: MyStruct) {
value.#^MEMBER_INSTANCE^#
// MEMBER_INSTANCE-DAG: Keyword[self]/CurrNominal: self[#MyStruct#]; name=self
// MEMBER_INSTANCE-DAG: Decl[InstanceMethod]/CurrNominal: instanceMethod()[#Void#]; name=instanceMethod()
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: instanceMethod_peer[#Int#]; name=instanceMethod_peer
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: staticMethod_peer[#Int#]; name=staticMethod_peer
// MEMBER_INSTANCE-DAG: Decl[InstanceMethod]/CurrNominal: forArbitrary()[#Void#]; name=forArbitrary()
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: forArbitrary_peer[#Int#]; name=forArbitrary_peer
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: foo[#Int#]; name=foo
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: addOne[#(Int) -> Int#]; name=addOne
// MEMBER_INSTANCE-DAG: Decl[InstanceVar]/CurrNominal: value[#Int#]; name=value
// NOTE: 'staticMethod_peer' is a instance var because the macro emits the decl without 'static'
}

func testMemberStatic() {
MyStruct.#^MEMBER_STATIC^#
// MEMBER_STATIC-NOT: _peer
// MEMBER_STATIC-DAG: Keyword[self]/CurrNominal: self[#MyStruct.Type#]; name=self
// MEMBER_STATIC-DAG: Keyword/CurrNominal: Type[#MyStruct.Type#]; name=Type
// MEMBER_STATIC-DAG: Decl[Struct]/CurrNominal: A[#MyStruct.A#]; name=A
// MEMBER_STATIC-DAG: Decl[Struct]/CurrNominal: B[#MyStruct.B#]; name=B
// MEMBER_STATIC-DAG: Decl[InstanceMethod]/CurrNominal: instanceMethod({#(self): MyStruct#})[#() -> Void#]; name=instanceMethod(:)
// MEMBER_STATIC-DAG: Decl[StaticMethod]/CurrNominal: staticMethod()[#Void#]; name=staticMethod()
// MEMBER_STATIC-NOT: _peer
}

func testLocal() {
#defineDeclsWithKnownNames

@PeerWithSuffix func localFunc() {}

do {
#^LOCAL?skip=FIXME^#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to an xfail so we don’t forget to change it if it’s fixed?

Suggested change
#^LOCAL?skip=FIXME^#
#^LOCAL?xfail=FIXME^#

Copy link
Member Author

@rintaro rintaro Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deliberately make this skip for now. There's two FIXMEs here.

  1. MacroExpansionExpr is not type-checked.
  2. Macro expansion in function bodies in fast-completion.

If we fix (1) but not (2) this test goes nondeterministic because it succeeds only when LOCAL token is tested first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. Makes sense.

// FIXME: macros in replace function bodies are not handled correclty.
// FIXME: decls instroduced by #defineDeclsWithKnownNames are missing.
// LOCAL-DAG: Decl[FreeFunction]/Local: localFunc()[#Void#]; name=localFunc()
// LOCAL-DAG: Decl[LocalVar]/Local: localFunc_peer[#Int#]; name=localFunc_peer
// LOCAL-DAG: Decl[Struct]/Local: A[#A#]; name=A
// LOCAL-DAG: Decl[Struct]/Local: B[#B#]; name=B
// LOCAL-DAG: Decl[LocalVar]/Local: foo[#Int#]; name=foo
// LOCAL-DAG: Decl[LocalVar]/Local: addOne[#(Int) -> Int#]; name=addOne
}
}
13 changes: 13 additions & 0 deletions test/Macros/Inputs/syntax_macro_definitions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1626,3 +1626,16 @@ public struct InitializableMacro: ConformanceMacro, MemberMacro {
return [requirement]
}
}

public struct PeerValueWithSuffixNameMacro: PeerMacro {
public static func expansion(
of node: AttributeSyntax,
providingPeersOf declaration: some DeclSyntaxProtocol,
in context: some MacroExpansionContext
) throws -> [DeclSyntax] {
guard let identified = declaration.asProtocol(IdentifiedDeclSyntax.self) else {
throw CustomError.message("Macro can only be applied to an identified declarations.")
}
return ["var \(raw: identified.identifier.text)_peer: Int { 1 }"]
}
}