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

Don't issue spurious nilary-overrides-nullary warning #10509

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Aug 25, 2023

fixes scala/bug#12851

My ambition had only been to fix the crasher stemming from #10484, but this fixes the underlying spurious-warning bug, too.

@som-snytt I gather from one of your other messages that you've already written a partest for this. So I didn't write one, on the assumption either that your own PR will supersede this PR, or that you'll kindly add your partest to this PR.

What I did do, though, was manually verify that it fixes what I reported in scala/bug#12851, as well as fixing it in the Ammonite repo where the problem originally turned up.

@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Aug 25, 2023
@SethTisue SethTisue added this to the 2.13.12 milestone Aug 25, 2023
@som-snytt
Copy link
Contributor

som-snytt commented Aug 25, 2023

The errors are supposed to go thru emitOverrideError so that if the member was inherited, the error is collected in mixinOverrideErrors and emitted at the end of the override checks using the class position instead. TIL.

(I'm sure I saw that in years past without understanding the why.)

I'll see if a PR is forthcoming in a few minutes by tomorrow.

I do see this fix shortcircuits, but if the traits are unrelated, you need to check the override of the composition.

@lrytz
Copy link
Member

lrytz commented Aug 25, 2023

The errors are supposed to go thru emitOverrideError so that if the member was inherited, the error is collected in mixinOverrideErrors and emitted at the end of the override checks using the class position instead. TIL.

Aha. TWL.

@som-snytt
Copy link
Contributor

is TWL a German thing? Treue Wörter Lauten

@SethTisue
Copy link
Member Author

probably to be superseded by #10512

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

I add the pos test from the other PR, and verified that it fails before.

This fix avoids the case that Seth noted shouldn't even warn: both "legs" of the override pair are compiled separately. The lower leg lacks a source position.

@som-snytt som-snytt marked this pull request as ready for review August 28, 2023 01:23
@som-snytt som-snytt merged commit 7444ea8 into scala:2.13.x Aug 28, 2023
4 checks passed
@SethTisue SethTisue removed the prio:blocker release blocker (used only by core team, only near release time) label Aug 28, 2023
@SethTisue SethTisue deleted the nullary-nilary-crashy-crashy branch August 28, 2023 02:31
@SethTisue SethTisue added internal not resulting in user-visible changes (build changes, tests, internal cleanups) and removed internal not resulting in user-visible changes (build changes, tests, internal cleanups) labels Aug 28, 2023
@SethTisue SethTisue changed the title Don't crash issuing spurious nilary-overrides-nullary warning Don't issue spurious nilary-overrides-nullary warning Aug 28, 2023
@som-snytt
Copy link
Contributor

som-snytt commented Aug 28, 2023

The word is not "scurrilous" but "scurrilary".

That's when you have a scurried function.

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