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

Restrict implicit lint to view #10781

Merged
merged 1 commit into from
May 22, 2024

Conversation

som-snytt
Copy link
Contributor

Only warn about a block arg passed into the by-name param of an implicit method if it was an implicit view.

Fixes scala/bug#12072

This implements lrytz's suggestion on the ticket.
scala/bug#12072 (comment)

Confirmed that the Shapeless 2 solutions to the guide exercises now compile cleanly.

Since Shapeless is not relying on conversions, this PR ignores the ticket title as a red herring.

@scala-jenkins scala-jenkins added this to the 2.13.15 milestone May 14, 2024
@som-snytt som-snytt marked this pull request as ready for review May 14, 2024 23:34
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.

LGTM

t match {
case Apply(coercion, _) if t.isInstanceOf[ApplyImplicitView] =>
coercion.symbol.paramLists match {
case (p :: Nil) :: _ if p.isByNameParam => refchecksWarning(t.pos, s"Block result was adapted via implicit conversion (${coercion.symbol}) taking a by-name parameter", WarningCategory.LintBynameImplicit)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case (p :: Nil) :: _ if p.isByNameParam => refchecksWarning(t.pos, s"Block result was adapted via implicit conversion (${coercion.symbol}) taking a by-name parameter", WarningCategory.LintBynameImplicit)
case (p :: Nil) :: _ if p.isByNameParam => refchecksWarning(t.pos, s"Block result was adapted via implicit conversion (${coercion.symbol}) taking a by-name parameter; statements are not evaluated eagerly", WarningCategory.LintBynameImplicit)

or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or just the opposite. This is the opposite where only the result expression is adapted. I'll upgrade both messages!

@som-snytt som-snytt force-pushed the issue/12072-lint-implicit-by-name branch from 9e4b360 to baa66b2 Compare May 21, 2024 15:45
@som-snytt
Copy link
Contributor Author

I pushed messages for the two cases that are less cryptic.

The OP confusing case is that if a block is converted, the conversion applies only to the result expression. This matters if the method parameter is by-name.

The related confusing case is that if the method is overloaded, the conversion applies to the whole block, the opposite of the first case.

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.

Thanks!

@lrytz lrytz merged commit 896427d into scala:2.13.x May 22, 2024
3 checks passed
@som-snytt som-snytt deleted the issue/12072-lint-implicit-by-name branch May 22, 2024 07:44
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants