chapel-py: fix "IDs available in scope" #24988
Conversation
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
The names of used modules appear via the imports that bring them in. Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
There was a problem hiding this comment.
Looks good. Could you please add a C++ test for getSymbolsAvailableInScope ? It'll be much easier to check that it's working correctly that way (make sure to check import Module.Item and use Module and use Module only Thing1 as Thing2). Also it'll be much easier to avoid future problems with the language server if we have a C++ test for this.
| const IdAndFlags::FlagSet& excludeFilter, | ||
| const Scope* scope) { | ||
| auto scopeAst = parsing::idToAst(context, scope->id()); | ||
| auto visibility = scopeAst->toDecl()->visibility(); |
There was a problem hiding this comment.
I know you are just moving this code, but could you fix this issue with it?
It should use parsing::idIsPrivateDecl to avoid calling idToAst which is both slow and unlikely to be reusable from revision to revision
There was a problem hiding this comment.
That doesn't quite work because the argument expects a Visibility, not a boolean. But I'll add a new parsing query to retrieve the visibility.
There was a problem hiding this comment.
AFAIK, we don't need to distinguish here between default visibility and public. You could just write
auto visibility = idIsPrivateDecl(context, scope->id()) ? Decl::PRIVATE : Decl::Public
Distinguishing between public and default visibility is useful in the uAST but I can't imagine any reason it would matter in scope resolution (and in fact, if it does matter, it's probably a bug, because the default visibility is public).
Of course you could put this in a helper in parsing-queries.
| if (kind == VisibilitySymbols::ONLY_CONTENTS || | ||
| kind == VisibilitySymbols::CONTENTS_EXCEPT) { | ||
| auto& namePairs = inVisibilitySymbols->names(); | ||
| // Should we even bother checking the names? |
There was a problem hiding this comment.
Maybe remove this comment? Checking the names seems reasonable to handle use X as Y etc. And it does check the names.
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
|
|
||
| const ModuleVec& vec = parseToplevel(context, path); | ||
|
|
||
| checkScopeContents(context, vec[0], {"x", "f1", "f2"}); |
There was a problem hiding this comment.
Arguably, it should also include M (but oddly enough, only for use in use / import). Also, arguably, it should include the included-by-default symbols. It's not important to me that it change behavior from what we have right now, but might be worth a comment here just to make updating the test easier.
Signed-off-by: Danila Fedorin <[email protected]>
The
doLookupInScopefamily of functions have special logic to introduce the symbol-to-be-imported into the search results. Specifically, use statements in the formuse IObringIOinto scope in addition the symbols fromIO. Prior to this PR,getSymbolsAvailableInScopedid not handle this, assuming that iteration overscope->declared()was sufficient. This PR addresses that problem.As a result, renaming modules now works properly with autocompletions in CLS, and the language server doesn't need to rely on
findUsedImportedIds.Reviewed by @jabraham17 and @mppf -- thanks!
Testing