SI-9383 Improved unused import warning#4616
Conversation
Previously, implicit search would mark every import it touched as a lookup. Instead, let subsequent type check perform the lookup.
|
@mrerrormessage (who found/reported the bug), maybe you could help review the test cases? |
|
I can't believe I got to fix an error message for @mrerrormessage! That's like scrubbing the kitchen floor and Mr Clean walks in. Now I understand why the people in those commercials look so happy. |
|
This definitely solves my case, and looks pretty thorough. The only case I can think of that isn't explicitly covered (although I don't know enough to know whether it ends up being covered anyhow) is the case where the implicit itself is not imported explicitly by the user a-la |
|
@mrerrormessage I commented on the ticket that it doesn't matter how implicit search was kicked off. The weird example was integer subtraction (of all things), because it's overloaded and overloading tries to convert between param types when deciding on a most specific alternative. Actually, I'll add both examples to the test, the string.contains was puzzler-worthy if it weren't also bug-worthy. I used them for testing until I noticed the existing unit test. |
|
Awesome. Thanks for fixing this! |
There was a problem hiding this comment.
so this method is only called from collectImplicitImports, in which case you want to have record = false. to me it seems a little bit more clear to push record = false out to the callsite within collectImplicitImports. the method name here (importedAccessibleSymbol) doesn't tell me why it should or should not record the import.
There was a problem hiding this comment.
I agree that the two code paths (lookupImport which is a real import versus importedAccessibleSymbol which is just a query) could have been better delineated in the original code, maybe by naming it isThisSymbolAccessiblyImportableAskingForAFriend, but the facade method was clearly intended to reduce clutter (of only one extra parameter). So the question is whether the facade method should be removed, the def at L953 renamed, and endowed with default args, for its one call site. Meh.
Since the query never looks for explicit import and never records, the params shouldn't be pushed to the callsite.
The facade method avoids defaults which would never be specified.
|
the rest looks great! |
Per review, adds a function that clarifies the purpose of the code path that collects imported implicits, namely, to examine qualifying imported implicits and then collect them. In the context of doing something with the import (besides importing), it's clear why we don't want to record that we used the import, that is, because we might be doing something other than using. That's clear, right?
SI-9383 Improved unused import warning
|
thanks everybody, especially @som-snytt |
|
Was this the PR that got me sucked into -Wunused? |
Previously, implicit search would mark every import
it touched as a lookup.
Instead, let subsequent type check perform the lookup.