Skip to content

Fix #8763: Limit number of import suggestions #8773

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

Merged
merged 2 commits into from
Apr 24, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 22, 2020

Limit number of import suggestions and make computing the
top-ranked suggestions more efficient.

Limit number of import suggestions and make computing the
top-ranked suggestions more efficient.

/** The best `n` references in `refs`, according to `compare`
* `compare` is a partial order. If there's a tie, we take elements
* in the order thy appear in the list.
Copy link
Contributor

Choose a reason for hiding this comment

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

One small typo: thy should probably be they.

@odersky odersky added the fasttrack Simple fix. Reviewer should merge or apply additional changes directly. label Apr 23, 2020
@odersky odersky added this to the 0.24.0-RC1 milestone Apr 23, 2020
@odersky
Copy link
Contributor Author

odersky commented Apr 23, 2020

Since it fixes a potential very long compilation, it would be good to have this in 0.24

@odersky odersky requested a review from julienrf April 23, 2020 10:28
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

That sounds reasonable. Should we also run the benchmarks?

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.

Otherwise LGTM

var i = 0
var diff = 0
while i < filled && diff == 0 do
diff = compare(ref, top(i))(using ctx.retractMode(Mode.ImplicitsEnabled))
Copy link
Member

Choose a reason for hiding this comment

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

Store ctx.retractMode(Mode.ImplicitsEnabled) in a val to avoid recomputing it at every iteration

Copy link
Contributor

Choose a reason for hiding this comment

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

@smarter could you apply this fix directly, as per fasttrack label policy?

@smarter
Copy link
Member

smarter commented Apr 23, 2020

Should we also run the benchmarks?

We don't have any benchmark infrastructure for code that contains compilation errors, so no way to benchmark this

@smarter smarter merged commit d94e1ec into scala:master Apr 24, 2020
@smarter smarter deleted the fix-#8763 branch April 24, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fasttrack Simple fix. Reviewer should merge or apply additional changes directly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants