Skip to content

Remove FindLocalVal #71357

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 2 commits into from
Feb 9, 2024
Merged

Remove FindLocalVal #71357

merged 2 commits into from
Feb 9, 2024

Conversation

hamishknight
Copy link
Contributor

@hamishknight hamishknight commented Feb 2, 2024 •
edited
Loading

Replace with an ASTScope lookup. This also lets us simplify UsableFilteringDeclConsumer, and fixes a couple of completion bugs.

Resolves #66925
Resolves #71384
rdar://92479209
rdar://108164277
rdar://122302481

@slavapestov
Copy link
Contributor

Yes!

@hamishknight hamishknight force-pushed the scoped-down branch 2 times, most recently from 543fb81 to e6ede36 Compare February 5, 2024 16:36
@hamishknight
Copy link
Contributor Author

@swift-ci please test

@hamishknight
Copy link
Contributor Author

@swift-ci please SourceKit stress test

@hamishknight hamishknight changed the title [DNM] Remove FindLocalVal Remove FindLocalVal Feb 5, 2024
@hamishknight hamishknight marked this pull request as ready for review February 5, 2024 16:38
hamishknight
hamishknight commented Feb 5, 2024
View reviewed changes
lib/Sema/LookupVisibleDecls.cpp
auto *SF = DC->getParentSourceFile();
assert(SF);
assert(Loc.isValid());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any objections to requiring a valid SourceLoc and SourceFile? We could potentially fall back to a module-scope lookup, but it's not clear to me how useful such a behavior would be (seems like callers ought to use lookupVisibleDeclsInModule if that's what they really want), and it seems like all the current clients meet this requirement.

Copy link
Member

Choose a reason for hiding this comment

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

No objection. Maybe we should even add

assert(DC->isModuleScopeContext());

because without IncludeTopLevel: true, that lookup doesn't make any sense. If the caller really wants only top-level decls, they usually should use ModuleDecl::lookupVisibleDecls()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presumably you mean assert(!DC->isModuleScopeContext());? Looks like that can be hit for e.g decl attribute completion, and I think seems reasonable to have a module scope context there

Copy link
Member

Choose a reason for hiding this comment

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

OK. But just out of curiosity, does swift::lookupVisibleDecls() at SourceFile context without IncludeTopLevel emit anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't include anything

@hamishknight
Copy link
Contributor Author

Same stress tester failures as main, except for TodayTurnipSection.swift at offset 1405, which is a soft timeout on main (although it's at 293 seconds, so it's pretty close to exceeding the 300 second timeout as-is, and testing locally I get a ~1s speedup with this patch from ~55s to ~54s).

rintaro
rintaro reviewed Feb 7, 2024
View reviewed changes
@hamishknight
NFC: Rename DeclVisibilityKind::LocalVariable -> LocalDecl
09ab888 
This includes local types, so make the name a bit
more generic.
@hamishknight
Remove FindLocalVal
f4b928f 
Replace with an ASTScope lookup. This also lets
us simplify UsableFilteringDeclConsumer, and fixes
a couple of completion bugs.
@hamishknight
Copy link
Contributor Author

@swift-ci please test

rintaro
rintaro approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

lgtm!

@hamishknight hamishknight merged commit e380c66 into swiftlang:main Feb 9, 2024
@hamishknight hamishknight deleted the scoped-down branch February 9, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rintaro rintaro rintaro approved these changes

@ahoppen ahoppen Awaiting requested review from ahoppen

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@CodaFi CodaFi Awaiting requested review from CodaFi CodaFi is a code owner

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

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

Successfully merging this pull request may close these issues.

Missing completion for bound variable in switch expression Static functions from outer type are not suggested
3 participants
@hamishknight @slavapestov @rintaro