Skip to content
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

Fix #3979: Completion for renamed imports #4977

Merged
merged 9 commits into from
Dec 2, 2018

Conversation

Duhemm
Copy link
Contributor

@Duhemm Duhemm commented Aug 21, 2018

We introduce a new RenameAwareScope that tracks the name associated
with a symbol in the scope. Calling toListWithNames returns the list
of symbols in this scope along with the name that should be used to
refer to the symbol in this scope.

Fixes #3979

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 1, 2018

Rebased

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 27, 2018

Rebased and took a stab at #5508

@Duhemm
Copy link
Contributor Author

Duhemm commented Nov 28, 2018

@smarter Thanks, I've addressed your comments.

One `Completion` represents one completion results that should be
included in the list of completion options. It can represent zero
symbols (for instance, a wildcard import), or more than one (a class and
its companion object). Each `Completion` also has a `description` which
should be displayed in the tool that shows the completion candidates.

Fixes scala#5508
We introduce a new `RenameAwareScope` that tracks the name associated
with a symbol in the scope. Calling `toListWithNames` returns the list
of symbols in this scope along with the name that should be used to
refer to the symbol in this scope.

Fixes scala#3979
For instance, Scala classes have a synthetic accompanying `Module` which
doesn't have a corresponding source symbol. We don't want to show the
`Module` in the completions.
When multiple symbols are concerned by one completion result, we show
the different kinds of symbols.
@Duhemm
Copy link
Contributor Author

Duhemm commented Dec 1, 2018

@smarter The NameFilter is back, with the fix that we found yesterday.

sym <- syms
} yield (sym, name)
/** Get the names that are known in this scope, along with the list of symbols they refer to. */
def mappings(implicit ctx: Context): Map[SimpleName, List[Symbol]] = {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't the body of this method just be equal to nameToSymbols ? (assuming def enter above is changed to do stripModuleClassSufffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch. That should have been the case from the beginning, since nameToSymbold contained exactly one symbol per name. Thanks


/** Enter the symbol `sym` in this scope, recording a potential renaming. */
def enter[T <: Symbol](sym: T, name: Name)(implicit ctx: Context): T = {
nameToSymbols += name -> (sym :: nameToSymbols.getOrElse(name, Nil))
val simpleName = name.stripModuleClassSuffix.toSimpleName
Copy link
Member

Choose a reason for hiding this comment

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

How come toSimpleName is needed ? Aren't we already filtering out all non-simple-names with the completionsFilter ?

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, completionsFilter filters names based on their kind (Simple, Qualified, Numbered and so on), which is a different distinction from SimpleName / TermName / TypeName as far as I can tell.

We need to do toSimpleName here so that TermNames and TypeNames are grouped.

Copy link
Member

@smarter smarter Dec 1, 2018

Choose a reason for hiding this comment

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

Ah, I see, I would do toTermName then since SimpleNames are TermNames anyway, and it means that we don't accidentally mangle encoded names.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

👍

@Duhemm Duhemm merged commit 7c7af27 into scala:master Dec 2, 2018
@Duhemm Duhemm deleted the fix/3979 branch December 2, 2018 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants