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

Turn given loop prevention on for -source future #19392

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 7, 2024

Drop the experimental language import. I believe everybody agrees that this is a desirable improvement, and type inference and implicit search have traditionally been in the realm of the compiler implementers.

Experimental language imports have to live forever (albeit as deprecated once the feature is accepted as standard). So they are rather heavyweight and it is unergonomic to require them for smallish improvements to type inference.

The new road map is as follows:

  • In 3.4: warning if behavior would change in the future.
  • In 3.5: error if behavior would change in the future
  • In 3.future (at the earliest 3.6): new behavior.

Drop the experimental language import. I believe everybody agrees that this
is a desirable improvement, and type inference and implicit search
have traditionally been in the realm of the compiler implementers.

Experimental language imports have to live forever (albeit as deprecated
once the feature is accepted as standard). So they are rather heavyweight
and it is unergonomic to require them for smallish improvements to type
inference.

The new road map is as follows:

 - In 3.4: warning if behavior would change in the future.
 - In 3.5: error if behavior would change in the future
 - In 3.future (at the earliest 3.6): new behavior.
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Do we want to make sure that fatal warning users have a way to upgrade? I would hope that it's possible to Wconf the warning away, but I'm not 100% confident. Should we double-check with a test?

LGTM for the rest

@odersky
Copy link
Contributor Author

odersky commented Jan 7, 2024

Suppressing the warning would only be a temporary fix since in 3.5 we emit an error in the same situation. The warning does give hints how to work around the problem, and the sooner people apply them the better. So I think we are as good as we could be.

@sjrd
Copy link
Member

sjrd commented Jan 7, 2024

Of course suppressing a warning is a temporary fix. But temporary fixes are good!

The typical scenario that users face is: they upgrade, they get plenty of new warnings-that-are-errors. These warnings come not only from this PR, but from all the other stuff that is newly a warning. What do they do?

  • If no Wconf escape:
    • Try to address everything at the same time -> may need a giant commit; hard to know the cause of breakage if they face any.
    • Disable fatal-warnings -> can do multiple commits, but it's hard to tell whether they've addressed all the issues of one kind, or whether fixes introduce new unexpected warnings in the meantime; cannot do multiple PRs because they understandably won't accept non-fatal-warnings in main
  • If Wconf escape is available:
    • Upgrade and Wconf-away all the new warnings. Commit, PR and merge. Then, progressively, with individual PRs and reviews, fix all the occurrences of each new kind of warning.

There is substantial value to users to having the ability to temporarily, selectively ignore new warnings. Even if it works only within a single minor release.

@odersky
Copy link
Contributor Author

odersky commented Jan 7, 2024

@sjrd OK. Can one of you suggest a WConf test or fix? I have not experience with that area.

@som-snytt
Copy link
Contributor

In Scala 2.13.13, these migratory behaviors are controlled by -Xsource:3 where the stages are warning, soft error, and then new behavior with no message about it.

-Xsource:3migration gives you a warning, -Xsource:3 issues the same message but with -Wconf that bumps it to error, and -Xsource:3cross introduces the new behavior. So the difference is between a "hard error" and a "warning configured as a soft error" so it can be detuned by -Wconf. Scala 2 doesn't have actual future versions because of the decision about 2.14, but if you want the new behavior, you don't get a warning about it (because you're living in the future). Short of that, you get warning or warning boosted to error.

One may joke that the alternative is to change all warnings to language imports.

@odersky
Copy link
Contributor Author

odersky commented Jan 8, 2024

I added a test, which shows that @nowarn works.

I am sure -Wconf would work also, since it's very general.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thank you!

@odersky odersky merged commit 65fca39 into scala:main Jan 8, 2024
19 checks passed
@odersky odersky deleted the std-glp branch January 8, 2024 12:10
@odersky odersky added this to the 3.4.0 milestone Jan 8, 2024
@Kordyjan Kordyjan added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jan 8, 2024
@WojciechMazur
Copy link
Contributor

WojciechMazur commented Jan 11, 2024

Question:
I wonder what should be behaviour under usage of -source:future-migration, should it fail similarly as under -source:future or give a warning? Right now it is failing
I've just found a project in OpenCB failing due to this change, which is expected now, but lack of context was confusing. Maybe under future-migration we should still produce a warning?

Kordyjan added a commit that referenced this pull request Jan 18, 2024
@WojciechMazur WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024
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.

6 participants