-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Parse][CodeCompletion] Completions for precedencegroup decls #17649
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
[Parse][CodeCompletion] Completions for precedencegroup decls #17649
Conversation
@jrose-apple Have I tagged the correct people to review this? |
Yep, those two seem fine! They'll tell you if it should be anyone else, too. |
@benlangmuir @nkcsgexi sorry to disturb, reminder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress overall! Some comments inline :)
include/swift/Syntax/TokenKinds.def
Outdated
/// Keywords that are endemic to specific contexts, i.e. precedence group decls. | ||
#ifndef CONTEXTUAL_KEYWORD | ||
#define CONTEXTUAL_KEYWORD(kw) SWIFT_KEYWORD(kw) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, could you avoid changing TokenKinds.def
file? We currently parse these contextual keywords as identifiers and we have logic to promote them from identifier to keywords. Changing their kinds here can have larger impact than being necessary. Also, we have other contextual keywords too, e.g. get
and set
; it's hard to be exhaustive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this would be a nice starting point to clean up all of the string literal checks and conversions from identifiers that contextual keywords are currently suffering. In addition, even the highlighting is broken on them, possibly as a consequence. Will this cause Keyword cannot be used as an identifier here
in unnecessary places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These contextual keywords can be used as regular Swift identifiers, i.e. the code snippet is entirely valid:
func foo() {
let get = 1
let associativity = 2
let higherThan = 3
...
}
However, the identifiers here are keywords in some circumstances. We have two ways to handle this bifurcation: (1) lexing them as identifier and later promote them during parsing to keywords, or (2) lexing them as contextual keywords and later demote them during parsing to identifier. The current implementation is using (1), and the change you made is changing it to (2). I cannot see any apparent benefit of using one over the other. So, that's why I suggest we avoid changing the kinds in TokenKinds.def
to keep things simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I agree, (1) is the most convenient.
lib/IDE/CodeCompletion.cpp
Outdated
} | ||
break; | ||
} | ||
default: return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move return
to another line.
lib/IDE/CodeCompletion.cpp
Outdated
} | ||
|
||
void getPrecedenceGroupCompletions(SyntaxKind SK) { | ||
#define AddKeyword(kw) addKeyword(getTokenText(tok::kw), None, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make it a closure or function call instead of a macro for debuggability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If twice as much space wasted is not a problem – of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, space should be fine. Readability and debuggability are more important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually thought it would be OK while it's defined next to the use cases. And it has a nice safety advantage: you won't mistakingly pass the wrong tok::kw
or CodeCompletionKeywordKind::kw
(they have to match).
utils/gyb_syntax_support/Token.py
Outdated
Keyword('Associativity', 'associativity'), | ||
Keyword('HigherThan', 'higherThan'), | ||
Keyword('LowerThan', 'lowerThan'), | ||
Keyword('Assignment', 'assignment'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, avoid adding these new keywords for consistency.
diagnoseOperatorFixityAttributes(*this, Attributes, res); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please separate all these whitespace and comment changes that aren't related to the functional change into a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew you would say so 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR or simply a commit will do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit is fine
lib/IDE/CodeCompletion.cpp
Outdated
@@ -2740,7 +2742,7 @@ class CompletionLookup final : public swift::VisibleDeclConsumer { | |||
addTypeAnnotation(Builder, EnumType); | |||
} | |||
|
|||
void addKeyword(StringRef Name, Type TypeAnnotation, | |||
void addKeyword(StringRef Name, Optional<Type> TypeAnnotation = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just pass a null Type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so that's the convention. I wasn't sure it was OK.
lib/IDE/CodeCompletion.cpp
Outdated
switch (FU->getKind()) { | ||
case FileUnitKind::SerializedAST: { | ||
SmallVector<Decl *, 16> topLevelDecls; | ||
cast<LoadedFile>(FU)->getTopLevelDecls(topLevelDecls); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterating over all the top-level symbols in all the imported modules is likely to be expensive. Could we instead add the precedence groups to the set of completions we cache per-module? Then we would request the cached values for each imported module and then only do the lookup in the current module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Roughly, where do we cache those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache is in CodeCompletionCache
and you request results from it using RequestedCachedModule
. Since you'll need a way to only include precedence groups and not all symbols from the module, look at how we use the flag OnlyType
inside that request; you could add an OnlyPredenceGroups
that works similarly.
The requests are put in Lookup.RequestedCachedResults
and then handled in handleResultsAndModules
and copyCodeCompletionResults
.
@benlangmuir I've hooked everything up, but apparently I still have to include precedence groups to what's cached per module, because they don't appear despite all other top-level declarations present in the cache sink. I don't see where this is regulated in |
@AnthonyLatsis We probably need to handle
|
@benlangmuir This appears to be a larger problem. I just realized Do we switch to |
@AnthonyLatsis Argh. Sorry for leading you astray. I doubt we want to change the visible decl consumer interface, so we'll have to separately lookup the PrecedenceGroupDecls when we fill the cache. In the worst case we could walk all the Decls like you were doing originally - at least this way we only do it once to fill the cache. @jrose-apple is there a better way to find all the PrecedenceGroupDecls in a module? Should we add a new API to build an array of these from the data in ModuleFile ? |
ae01860
to
7880b75
Compare
Some improvisation. Works fine, at least with serialized modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really happy with the overall change now and this seems close to ready. Thanks for working through the caching change!
@@ -2013,6 +2013,16 @@ void ModuleFile::getTopLevelDecls(SmallVectorImpl<Decl *> &results) { | |||
} | |||
} | |||
|
|||
void ModuleFile::getPrecedenceGroups( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a pretty stack trace to the beginning of the function like we do in e.g. getTopLevelDecls
so that if deserialization crashes we see this information.
PrettyStackTraceModuleFile stackEntry(*this);
lib/IDE/CodeCompletion.cpp
Outdated
CodeCompletionDeclKind::PrecedenceGroup) { | ||
return true; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified to return R->getAssociatedDeclKind() == CodeCompletionDeclKind::PrecedenceGroup
.
lib/Parse/ParseDecl.cpp
Outdated
@@ -6594,21 +6610,39 @@ Parser::parseDeclPrecedenceGroup(ParseDeclOptions flags, | |||
// We want to continue parsing after this. | |||
invalid = true; | |||
} | |||
attrKeywordLoc = consumeToken(tok::identifier); | |||
attrKeywordLoc = consumeToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I changed it to the shortcut version because we only call this lambda after we make sure the tok's an identifier, but yes, it isn't related to this PR and it's safer with the check, so I'll revert that.
lib/Parse/ParseDecl.cpp
Outdated
bool isLowerThan = false; | ||
if (attrName == "higherThan" || | ||
(isLowerThan = (attrName == "lowerThan"))) { | ||
if (attrName == "higherThan" || attrName == "lowerThan") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to change this? Moving this code adds an extra string comparison (admittedly it might not matter, but it doesn't seem related to this patch).
lib/Parse/ParseDecl.cpp
Outdated
diagnose(Tok, diag::unknown_precedencegroup_attribute, attrName); | ||
|
||
diagnose(Tok, diag::unknown_precedencegroup_attribute, Tok.getText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change from attrName
? It ought to be the same value, but using attrName
is less likely to break if someone refactors this code.
lib/IDE/CodeCompletion.cpp
Outdated
PrecedenceGroupDecl *group; | ||
if ((group = pair.second.getPointer()) && | ||
(pair.second.getInt() || includePrivate)) | ||
addPrecedenceGroupRef(group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we call getPrecedenceGroups()
to avoid duplicating this logic?
// ASSIGNMENT: Keyword[true]/None: true; name=true | ||
|
||
// PRECEDENCE_GROUP: Begin completions | ||
// PRECEDENCE_GROUP: Decl[PrecedenceGroup]/OtherModule[Swift]: BitwiseShiftPrecedence; name=BitwiseShiftPrecedence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking all the decls from the stdlib, I suggest just checking one or two (e.g. Assignment and Comparison). We prefer not to rely too much on stdlib details in code-completion tests.
// PRECEDENCE_GROUP: Decl[PrecedenceGroup]/OtherModule[Swift]: MultiplicationPrecedence; name=MultiplicationPrecedence | ||
// PRECEDENCE_GROUP: Decl[PrecedenceGroup]/OtherModule[Swift]: FunctionArrowPrecedence; name=FunctionArrowPrecedence | ||
|
||
// PRECEDENCE_GROUP_CURR_FILE-DAG: Decl[PrecedenceGroup]/CurrModule: MyPrecedence1; name=MyPrecedence1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the current file results separated? Shouldn't you get all the precedence groups in the stdlib and current file every time?
And we should probably have a test for a precedence group in another source file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I was having trouble getting precedence groups that are declared after the current one, they simply didn't show up. I thought this was happening because the rest hadn't been parsed yet and that's ok, but it doesn't really make sense now. I'll let you know if the issue is still present.
And we should probably have a test for a precedence group in another source file.
It's a good idea, but I'll need a hint on how to write a multifile test. Do you have to manually create a module for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having trouble getting precedence groups that are declared after the current one, they simply didn't show up.
Okay, we should fix that. If it turns out to be hard to fix I'm okay with taking this change without it though.
I'll need a hint on how to write a multifile test. Do you have to manually create a module for that?
No need for another module, just another file in the current module. You can add another file with a precedence group in the test/IDE/Inputs
directory and then add that file to the command line invocation of swift-ide-test on the RUN line. See complete_multiple_files.swift
for an example of how to do that.
Hey! Sorry for the delay, let's get this finished. |
7880b75
to
30ee315
Compare
All that's left is this:
I'll try an analogous query for operator declarations; hopefully the logic or parsing behind that'll give me an idea of what's going on. |
lib/Parse/ParseDecl.cpp
Outdated
// Skips the CC token if it comes without spacing. | ||
auto skipPostfixCodeCompletionToken = [&](char lastTokTextEnd) { | ||
if (Tok.is(tok::code_complete) && lastTokTextEnd != ' ' && | ||
lastTokTextEnd != '\t' && lastTokTextEnd != '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works because Tok.getText()
doesn't contain white spaces.
Usually, to determine any whitespaces precedes a token, getEndOfPreviousLoc() != Tok.getLoc()
is sufficient.
Also, if we skip code_complete
token, we should return hasCodeCompletion()
result even if it's ignored because further parsing is meaningless.
So I suggest:
auto skipPostfixCodeCompletionToken = [&] () -> Bool {
if (Tok.is(tok::code_complete) && getEndOfPreviousLoc() != Tok.getLoc()) {
conumeToken();
return true;
}
return false;
}
...
if (skipPostfixCodeCompletionToken())
return abortBody(/*hasCodeCompletion*/true);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is this really needed in the first place? Although
precedencegroup MyPrecedence {
associativity: right higherThan: AdditionPrecedence
}
is valid syntax, consecutive attributes in a single line is not a good style IMO.
Something like:
while (Tok.isNot(tok::r_brace)) {
if (Tok.is(tok::code_completion)) {
// Suggest attribute names only if CC token is at the start of line.
if (Tok.isAtStartOfLine() && CodeCompletion) {
CodeCompletion->completePrecedenceGroup(...)
}
consumeToken(tok::code_complete);
continue;
}
...
without skipPostfixCodeCompletionToken()
is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placing the check before parsing the next attribute is a good move, though I think I've already tried and abandoned it for some reason.
But, I believe that syntactic correctness is a superior criteria than 'bad style' for the compiler to support you.
Usually, to determine any whitespaces precedes a token,
getEndOfPreviousLoc() != Tok.getLoc()
is sufficient. ...if (Tok.is(tok::code_complete) && getEndOfPreviousLoc() != Tok.getLoc())
Does getEndOfPreviousLoc() != Tok.getLoc()
mean that there is or isn't a (whitespace
| \n
| \r
)? Intuitively, the locations not matching tells me there is something in between, but not necessarily a symbol we are interested in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between two adjacent tokens, only possible characters are whitespaces, newlines, comments, or other garbage characters treated as whitespaces (e.g. U+00A0 No-Break Space
). Since comments are treated as a whitespace:
precedencegroup MyPrecedence {
assignment: true/*comment*/higherThan: AssignmentPrecedence
}
is valid. So just checking (whitespace
| \n
| \r
) is insufficient if we really want to check "whitespace"s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was actually just wondering if the second part of the condition from your reply had a typo (should be getEndOfPreviousLoc() == Tok.getLoc()
)
if (Tok.is(tok::code_complete) && getEndOfPreviousLoc() != Tok.getLoc()) {
conumeToken();
return true;
}
Thanks for the tip on the preferred way of doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
getEndOfPreviousLoc() == Tok.getLoc()
Ah sorry. You are absolutely right!
associativity: none | ||
assignment: false | ||
lowerThan: AdditionPrecedence | ||
higherThan: #^PRECEDENCE_GROUP_3^# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test case for higherThan: SomePrecedence, #^TOKEN^#
30ee315
to
dcf7213
Compare
|
dcf7213
to
680db06
Compare
@benlangmuir Finally got my hands on this. Fixed the delayed parsing issue. |
Have a look at |
680db06
to
96d0f2a
Compare
lib/IDE/CodeCompletion.cpp
Outdated
case SyntaxKind::PrecedenceGroupAssociativity: | ||
addKeyword("none"); | ||
addKeyword("left"); | ||
addKeyword("right"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you refactor this mapping to a utility function like here? So other tooling can get the spelling as well. You can have a different commit to do that.
addKeyword("associativity"); | ||
addKeyword("higherThan"); | ||
addKeyword("lowerThan"); | ||
addKeyword("assignment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Associativity
is pretty straight-forward, but we don't have an enum or something of the like for precedence group attributes, so I suppose there isn't any tooling depending on the spelling apart from individual cases that don't require switching on a specific PG attribute.
Sorry for the slow reply! I missed the last notification. Thanks for tracking down what's going on with the forward reference case, this is actually a bigger issue than precedence groups! The real problem here is that Since this affects unrelated completions like inheritance, fixing it belongs in a separate PR and you can just ignore this bug for this one. |
I see, interesting. I'll revert those changes then and address the latest review comments.
|
Yeah, it's just the top-level scope of the module. |
Added the 'Module::getPrecedenceGroups' API to separate precedence group lookup from 'Module::lookupVisibleDecls', which together with 'FileUnit::lookupVisibleDecls', to which the former is forwarded, are expected to look up only 'ValueDecl'. In particular, this prevents completions like Module.PrecedenceGroup.
96d0f2a
to
d2e7c78
Compare
I believe all comments have been addressed, what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicted with #19756. LGTM otherwise. |
Resolved. |
@swift-ci Please smoke test |
1 similar comment
@swift-ci Please smoke test |
Thank you! |
@rintaro, ever since I started contributing to the compiler I have been regularly receiving Swift-CI build failure notifications after merges. At first I thought it's an indication of me messing up things, but then I realized it could as well be other people who recently committed, or even something older. The tests that fail are often completely unfamiliar to me. This PR is not an exception, but I would like to ask whether there's anything else I should know about these notifications. I already know you can't reply to start a conversation :) |
Yeah, that is not your fault. Sorry for the noise.
See: https://forums.swift.org/c/development/ci-notifications |
Thanks! No worries about the noise, I don't mind getting them at all, just curious. |
Completions for precedence group contexts and precedence groups in operator declarations.
Resolves SR-8043.