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

Get linting information from Typer instead of recreating its logic #18400

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Aug 14, 2023

Fixes #18366
Lays the groundwork to use the information from Typer in linting instead of recalculating it (e.g. isInImport in WUnused).

@szymon-rd szymon-rd marked this pull request as draft August 14, 2023 16:22
@szymon-rd szymon-rd force-pushed the keep-directly-imported-idents branch from 2ef4c6f to b4b5b9d Compare August 14, 2023 16:32
@szymon-rd szymon-rd changed the title Keep directly imported idents - fix linting bug Keep directly imported idents, fix linting bug Aug 14, 2023
@szymon-rd szymon-rd force-pushed the keep-directly-imported-idents branch from b4b5b9d to e25d0fd Compare August 16, 2023 11:51
@odersky
Copy link
Contributor

odersky commented Aug 21, 2023

I am very dubious about complexifying Typer for non-essential things like warnings. Complexity in all of these dimensions:

  • concerns that need to be addressed
  • runtime speed and allocation rate
  • complexity of the code itself

Is there really no other way? In my mind, even a ~5 times complexity increase elsewhere is a fair tradeoff against an increase of complexity in Typer, simply because in Typer we are already close to the thresholds of what can be understood, and what is a reasonable runtime complexity.

This is because if the complexity is elsewhere people can be trained to deal with it. But if it is in Typer it becomes a bottleneck for the few people who are confident in changing Typer.

I noted that is PR involves additional allocations in findRefs, which has previously been carefully optimized to avoid all such allocations.

@szymon-rd szymon-rd changed the title Keep directly imported idents, fix linting bug Get linting information from Typer instead of recreating its logic Aug 21, 2023
@szymon-rd
Copy link
Contributor Author

szymon-rd commented Aug 21, 2023

To get a clear picture, let's list the benefits that we get from extracting information from Typer:

  • Avoid duplicate logic & reduce the complexity of linting (no need to compare refs and imports in non-linear time). Therefore, likely - a significant speed-up of the linting. Assuming that most people use it most of the time, it should optimize compile times in many cases. Without approximating Typer's logic, maintenance of the linting will be much easier (especially beneficial if we manage to implement it as we planned, in logging/event log manner that does not influence the readability and maintainability of the Typer).
  • Fixing False warning about unused import for local/relative import with Scala 3.3 #18366 (information is otherwise lost, we can introduce new false negatives - i.e., miss reporting some unused imports)
  • Fix false positives:
  • Avoid false negatives:
    • when the import is used only in a transparent inline (when code would work without the import)
    • when the imported ref is a transparent inline (at this moment - skipped)
    • when name contains $ sign
    • introduced by marking all prefixes of types as used (introduced to fix -Wunused:all and nested type class derivation #16679)
    • introduced by heuristics in isInImport method - e.g. no differentiation between alias and dealiased type names when using derivation.
  • Some false negatives can cause false positives - when the wrong import is marked as used, the correct one will be unused.
  • If we trace information not only about imports, but also references in general, then we should fix most of other reported issues.

This implementation will be changed to one that uses an event logging pattern to avoid increasing the complexity of the typer. This way, Typer's code may become more readable in some places.

@som-snytt
Copy link
Contributor

The time to choose "let typer tell me" was with my PR for unused imports, which was relatively simple, namely, record when an import selector is used for lookup. All the complexity in CheckUnused is due to imports checking (because it requires contexts and scopes). Everything else (correlating definitions and references) is easy because it's just symbols.

I've removed the "internal traverser" from CheckUnused and used the miniphase API instead; I propose next to eliminate the stacks used to approximate scopes in favor of proper Contexts. (I did a kludge to handle args to parent constructors that happened to work; I thought fixing that would require the full rewrite.)

The advantage of keeping as much as possible in a separate phase is that it helps harden all behaviors. So although I would have preferred an instrumented compiler approach, now I see the benefit of not having code comments that explain "sbt needs this" or "the presentation compiler" or "linting".

At least CheckShadowing is in the same megaphase now; it's natural to want a linting megaphase where I can add a lint and rely on Context, perhaps with other linting info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False warning about unused import for local/relative import with Scala 3.3
3 participants