Skip to content

Warn if implicit default shadows given #23559

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jul 17, 2025

Fixes #23541

When supplying a default arg for an implicit application, warn if there is an implicit value available.

That is for f(using x = v) where a default is supplied in f(using x = v, y = default). The user might think the implicit value is supplied, which is not the case for explicit using; this is especially confusing in a recursive call, where the user intends to "replace" an implicit parameter.

Separately, behind a flag, warn if any recursive call uses a default arg instead of passing the current parameter value.

"Wtailrec-default", "Warn when a tail-recursive call uses a default arg."

@som-snytt
Copy link
Contributor Author

I didn't think to bootstrap; there are many instances, including the classic jsig!

[warn] 250 |            jsig(defn.ObjectType)
[warn]     |            ^^^^^^^^^^^^^^^^^^^^^
[warn]     |Argument for parameter toplevel was supplied using a default argument.

@som-snytt
Copy link
Contributor Author

Could offer a quickfix that inlines the default.

@som-snytt som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from 4bb349c to 9e53b78 Compare July 18, 2025 04:06
@odersky
Copy link
Contributor

odersky commented Jul 18, 2025

I think this warning would be too noisy. Implicit defaults are there for a reason, we do not want to warn each time we use one.

Also the change set looks too big and risky for such a relatively minor improvment.

@bracevac
Copy link
Contributor

It would be interesting to see how frequently this warning pops up in the open community build to have an understanding of how frequently these situations occur.

@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 18, 2025

The recursion case is the Scala 2 lint. I find it very confusing for the reason cited on the ticket: was this intentional or an error? The compiler can't help you decide, but you feel better about it if someone had to type in p = true. The expectation is that such "config" is passed down the stack.

For the using case, it tries to warn only if a default was used when an implicit would have been supplied from scope; not sure I did that the best way, but how often is that actually intended?

In fact, it might be cleaner to refuse to supply defaults in those cases. However, I haven't spent time thinking abstractly about it.

Noisy warnings are usually behind a flag; I wasn't sure if the compiler team wants to go that route, since flags (even for warnings) are also a maintenance burden and a challenge for cross-builds (especially under -Werror).

@som-snytt
Copy link
Contributor Author

This was the highlight of the effort.
image

Probably that is an ad for the mechanism described in the ticket to explicitly supply remaining implicits.

@som-snytt
Copy link
Contributor Author

Not sure about indirect recursion (where Scala 2 warns for the same reason)

class C {
  def f(x: Int, s: String = ""): List[Int] = {
    List(new C).flatMap(_.f(42))
  }
}

For fun, I'll try doing the check at tailrec or tailcalls whatever.

@som-snytt
Copy link
Contributor Author

Similar to the Scala 2 lint for "named boolean parameters" where you must name them if necessary to avoid ambiguity or accidentally switching args, one boolean parameter with a default might by OK (where there is one use site which requires the alternate behavior or state) but more than one is hard to follow, especially when recursing.

But that sounds like a third-party lint. (The named boolean lint was added as a project-internal lint on Scala 2.)

Implicit primitives add extra eye blinking.

@som-snytt som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from 9e53b78 to f038825 Compare July 18, 2025 16:20
@som-snytt
Copy link
Contributor Author

Put the recursion under a flag, but leave the titular warning.

I didn't retry the scaladoc test, which probably requires the "special syntax" for a satisfying fix.

@som-snytt
Copy link
Contributor Author

As suspected, the scaladoc case is noticed, appropriately.

[E219] Type Warning: /__w/scala3/scala3/scaladoc/src/dotty/tools/scaladoc/tasty/TypesSupport.scala:272:66

I will nowarn it for now, rather than munging the code.

@som-snytt som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from f038825 to c7045ea Compare July 18, 2025 17:03
@odersky
Copy link
Contributor

odersky commented Jul 18, 2025

The recursion case is the Scala 2 lint. I find it very confusing for the reason cited on the ticket: was this intentional or an error? The compiler can't help you decide, but you feel better about it if someone had to type in p = true. The expectation is that such "config" is passed down the stack.

I overlooked that. Yes, this is indeed confusing and should be avoided.

@som-snytt som-snytt marked this pull request as ready for review July 18, 2025 22:51
@som-snytt som-snytt force-pushed the issue/23541-warn-tricky-defaults branch from c7045ea to b144877 Compare July 20, 2025 02:07
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.

Add a lint/warning for the Interaction between using and default parameters
3 participants