Emit feature warnings for implicit conversions. #4229
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We now emit feature warnings when encountering an implicit conversion def, just like Scala 2 does. In addition, we also emit a feature warning when an implicit conversion is used, unless the conversion is an implicit class, or otherwise co-defined with the type to which it converts, or the conversion is predefined. Predefined are - all members of `Predef`, - the `scala.reflect.Selectable.reflectiveSelect` conversion. We might need to extend this to more conversions. The motivation for going down this route is the following: Implicit conversions are easily the most mis-used feature in Scala. If I should venture a guess I'd say that more than 90% of the uses are mis-guided. The danger of implicit conversions is that they seem simple. It's easy to grok what they are and in particular new users see the possibilities before realizing the downsides. So far we put definitions of implicit conversions under a feature flag but this is not enough. Library writers tend not to be deterred by it, they just add the language import. (Or, worse, IntelliJ does it for them. Dear IntelliJ Scala team: please change things so that language imports are not inserted automatically, They are required for a reason, and automatic insertion does not help!) Either way, it's the users of a library with implicit conversions that need to be warned by the compiler, since it is them who will run into troubles if implicit conversions have surprising behavior. In what concerns the precise rules, I believe we will have to try this out in the wild for a while to see how it shapes up and whether we should tweak the rules.
dottybot
reviewed
Apr 2, 2018
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
allanrenucci
approved these changes
Apr 20, 2018
| sym.is(Synthetic) || | ||
| sym.info.finalResultType.classSymbols.exists(_.owner.isLinkedWith(sym.owner)) || | ||
| defn.isPredefClass(sym.owner) || | ||
| sym.name == nme.reflectiveSelectable && sym.maybeOwner.maybeOwner.maybeOwner == defn.ScalaPackageClass |
There was a problem hiding this comment.
Why not compare symbol here?
There was a problem hiding this comment.
We don't want to force loading the symbol.
|
I don't think there is no need to warn the user when an implicit conversion function is imported separately. In this case, the user clearly expresses that he needs to use implicit conversion.. |
odersky
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 9, 2019
Implement feature warnings for implicit conversion use according to scala#4229 also for new style implicit conversions. Change the doc comment for languge.implicitConversion to explain why it is needed for use site conversions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We now emit feature warnings when encountering an implicit conversion def,
just like Scala 2 does.
In addition, we also emit a feature warning when an implicit conversion is used,
unless the conversion is an implicit class, or otherwise co-defined with the type
to which it converts, or the conversion is predefined.
Predefined are
Predef,scala.reflect.Selectable.reflectiveSelectconversion.We might need to extend this to more conversions.
The motivation for going down this route is the following: Implicit conversions are easily the
most mis-used feature in Scala. If I should venture a guess I'd say that more than 90% of the
uses are mis-guided. The danger of implicit conversions is that they seem simple. It's easy
to grok what they are and in particular new users see the possibilities before realizing the
downsides. So far we put definitions of implicit conversions under a feature flag but this is
not enough. Library writers tend not to be deterred by it, they just add the language import.
(Or, worse, IntelliJ does it for them. Dear IntelliJ Scala team: please change things so that
language imports are not inserted automatically, They are required for a reason, and automatic
insertion does not help!)
Either way, it's the users of a library with implicit conversions that need to be warned
by the compiler, since it is them who will run into troubles if implicit conversions have
surprising behavior.
In what concerns the precise rules, I believe we will have to try this out in the wild for a
while to see how it shapes up and whether we should tweak the rules.