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 messages leaking via suspended messages #13911

Merged
merged 1 commit into from Nov 10, 2021

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Nov 8, 2021

It isn't clear what StoreReporter's purpose is. Is it simply a store of
all messages, or is it a store of "real" messages, i.e. messages that
aren't suppressed with @nowarn or -Wconf (i.e. configurable warnings)?
I believe StoreReporter is often used as a way to get programmatic
access to the real messages.

Typer, with its TyperState, uses StoreReporter as a more general buffer
while making typing attempts, such as when trying to apply an implicit
conversion. But with configurable warnings, we don't know if a warning
is real or not until we've typed all the @nowarn annotation in the
code, which is why we suspend the messages, on Run.

So we add an override for TyperState, so that StoreReporter is used as
simply a buffer. When it then unbuffers its messages in flush to the
parent context's reporter, it will run through the regular
"issueIfNotSuppressed" code (assuming it's not another store reporter).

[test_java8]

@lrytz
Copy link
Member

lrytz commented Nov 8, 2021

For reference, in Scala 2, reports during type checking go through an additional layer (ContextReporter, separate class hierarchy from Reporter). Silent type mode for inference uses the BufferingReporter subclass of ContextReporter. This happens before any of the WConf/nowarn infrastructure looks at messages.

Scala 2 also has a StoreReporter, which is used for when messages should be made available as data instead of going to the console.

Scala 3's StoreReporter is used for both of these use cases. The bug is that the WConf infra can itself buffer messages (into the Run). This is needed because @nowarn suppression ranges are only known after typer. In silent mode / inference, messages buffered in the Run don't make it to the StoreReporter's buffer, so they are not discarded when an implicit alternative is discarded.

I guess an alternative fix would be to make removeBufferedMessages also handle messages buffered in the Run during the lifetime of that StoreReporter.

It isn't clear what StoreReporter's purpose is.  Is it simply a store of
all messages, or is it a store of "real" messages, i.e. messages that
aren't suppressed with `@nowarn` or -Wconf (i.e. configurable warnings)?
I believe StoreReporter is often used as a way to get programmatic
access to the real messages.

Typer, with its TyperState, uses StoreReporter as a more general buffer
while making typing attempts, such as when trying to apply an implicit
conversion.  But with configurable warnings, we don't know if a warning
is real or not until we've typed all the `@nowarn` annotation in the
code, which is why we suspend the messages, on Run.

So we add an override for TyperState, so that StoreReporter is used as
simply a buffer.  When it then unbuffers its messages in flush to the
parent context's reporter, it will run through the regular
"issueIfNotSuppressed" code (assuming it's not another store reporter).
@dwijnand dwijnand marked this pull request as ready for review November 8, 2021 20:30
@dwijnand dwijnand requested a review from lrytz November 8, 2021 20:32
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

It seems that there's a slight change; ctx.settings.silentWarnings.value used to be checked before WConf, now it's after. So before, silentWarnings would silence any warning, now it only silences warnings that WConf doesn't turn into errors.

To me the change is fine, it's probably the better way to do it.

@dwijnand dwijnand merged commit e23f815 into scala:master Nov 10, 2021
@dwijnand dwijnand deleted the fix-leaky-tryImplicits branch November 10, 2021 21:58
@dwijnand
Copy link
Member Author

Yeah... I can't remember why I made that change. I don't really know what the use cases are for -nowarn.

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.

Nightly Dotty workflow of 2021-11-04 failed
2 participants