-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
For reference, in Scala 2, reports during type checking go through an additional layer ( Scala 2 also has a Scala 3's I guess an alternative fix would be to make |
95f0d5d
to
31d6b99
Compare
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).
31d6b99
to
e5201a0
Compare
There was a problem hiding this 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.
Yeah... I can't remember why I made that change. I don't really know what the use cases are for |
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 thecode, 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]