Stabilise returned completions by improving deduplication + extra completions for constructors#19976
Merged
rochala merged 10 commits intoscala:mainfrom Apr 12, 2024
Conversation
kasiaMarek
reviewed
Mar 19, 2024
Comment on lines
+53
to
+54
| """|TestClass test.Wrapper (Module) | ||
| |TestClass(x: Int): TestClass (Method) |
Member
There was a problem hiding this comment.
The module seems far less useful than method-constructor. I'd prefer we either not show the module if it's not explicitly defined by the user (rarely useful) or show it after TestClass(x: Int).
Contributor
Author
There was a problem hiding this comment.
I also thought about removing synthetic modules, but in the end they are still valid symbols in this context.
I like the idea of adding higher precedence for applies if there is synthetic companion.
kasiaMarek
reviewed
Apr 10, 2024
kasiaMarek
reviewed
Apr 11, 2024
| denot :: applyDenots | ||
| if shouldAddSnippet && isNew && hasNonSyntheticConstructor then | ||
| sym.info.member(nme.CONSTRUCTOR).allSymbols.map(_.asSingleDenotation) | ||
| .filter(_.symbol.isAccessibleFrom(denot.info)) |
Member
There was a problem hiding this comment.
I'm not sure that you're filtering using the correct scope here (also line 254).
Contributor
Author
There was a problem hiding this comment.
I've added a test that checks if private members are correctly filtered, and they are failing without this line
In my opinion we're filtering with correct scope, as denot.info is the type in a specific call site.
kasiaMarek
approved these changes
Apr 12, 2024
Comment on lines
+1630
to
+1631
| """|fooBar(n: Int): Int | ||
| |fooBar: List[Int] |
Member
There was a problem hiding this comment.
Maybe a follow up? I don't think that here the apply completion should exist here and if so it shouldn't be first.
Contributor
Author
There was a problem hiding this comment.
I rebased the PR + fixed this, if you could give it one final look before we merge it.
04b3a77 to
bc7c848
Compare
kasiaMarek
approved these changes
Apr 12, 2024
WojciechMazur
added a commit
that referenced
this pull request
Jul 5, 2024
…pletions for constructors (#19976) This PR doesn't address all issues. In the future we need to get rid of https://github.com/scala/scala3/compare/main...rochala:improved-deduplication-and-constructors-search?expand=1#diff-035851592480495dfdb20da6b615ec7dd77b3db70cda46aba56230d8cd690773R157-R167 as it is not working as intended. The future PR of shortened type printer refactor includes a proper way to split extension params. `CompletionValue.Interpolator` should also be removed, and instead we should return workspace / completions as it is now hard to sort those completions. Next refactor is reusing completion affix for other kinds of completions such as case completions, so prefix / suffix is handled in single place. This PR will unblock fuzzy search in the compiler because of stabilizing returned completions. [Cherry-picked 97313ed][modified]
WojciechMazur
added a commit
that referenced
this pull request
Jul 5, 2024
…pletions for constructors (#19976) This PR doesn't address all issues. In the future we need to get rid of https://github.com/scala/scala3/compare/main...rochala:improved-deduplication-and-constructors-search?expand=1#diff-035851592480495dfdb20da6b615ec7dd77b3db70cda46aba56230d8cd690773R157-R167 as it is not working as intended. The future PR of shortened type printer refactor includes a proper way to split extension params. `CompletionValue.Interpolator` should also be removed, and instead we should return workspace / completions as it is now hard to sort those completions. Next refactor is reusing completion affix for other kinds of completions such as case completions, so prefix / suffix is handled in single place. This PR will unblock fuzzy search in the compiler because of stabilizing returned completions. [Cherry-picked 97313ed][modified]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR doesn't address all issues.
In the future we need to get rid of https://github.com/scala/scala3/compare/main...rochala:improved-deduplication-and-constructors-search?expand=1#diff-035851592480495dfdb20da6b615ec7dd77b3db70cda46aba56230d8cd690773R157-R167 as it is not working as intended. The future PR of shortened type printer refactor includes a proper way to split extension params.
CompletionValue.Interpolatorshould also be removed, and instead we should return workspace / completions as it is now hard to sort those completions.Next refactor is reusing completion affix for other kinds of completions such as case completions, so prefix / suffix is handled in single place.
This PR will unblock fuzzy search in the compiler because of stabilizing returned completions.