SI-8667 Improve too-many-args message

[som-snytt]

Use removeNames to help diagnose the application.
Supplement the error message with how many extra
args and any other residual assignments that the
user might have thought was a properly named arg.

[janekdb]

This is an improvement but 25 too many arguments for method could be regarded as the the more convoluted precursor to 25 more arguments than expected for method.

Compare,

A	B
one too many arguments for method	one more argument than expected for method
2 too many arguments for method	2 more arguments than expected for method
11 too many arguments for method	11 more arguments than expected for method
25 too many arguments for method	25 more arguments than expected for method

[som-snytt]

I think "expected" isn't the right word, because of defaults and tupling.

25 more arguments than can be applied to method f: (i: Int)Int

[janekdb]

That formulation looks good.

[som-snytt]

I'll miss the colloquial terseness of "too many args", and why did it even spell out "arguments", but thanks for the nudge.

[som-snytt]

I didn't squash, in case someone prefers this terser version:

t8667.scala:13: error: too many args (4) for constructor C: (a: Int, b: Int)C ; c, d are not parameters

//reduces counting but demands mental subtraction
t8667.scala:13: error: too many args (4 supplied but requires 2) for constructor C: (a: Int, b: Int)C ; c, d are not parameters

// instead of now
t8667.scala:13: error: 2 more arguments than can be applied to constructor C: (a: Int, b: Int)C ; c, d are not parameters

[som-snytt]

I squashed with an update for gradual messages that depend on the arg count. Off by one for a short param list is different from off by 5 for a 22-arity generated function.

Also made the "not a param name" message the same as for missing args.

[retronym]

Here's the current Javac version for comparison's sake:

% cat sandbox/Test.java  && javac sandbox/Test.java
class Test {
    void foo(int a) {};
    void bar() {
        foo(1, 2, 3);
    }
}
sandbox/Test.java:4: error: method foo in class Test cannot be applied to given types;
        foo(1, 2, 3);
        ^
  required: int
  found: int,int,int
  reason: actual and formal argument lists differ in length
1 error

[retronym]

An alternative (or additional ) idea: should be focus the error message at the first superfluous argument, rather than at the invoked method.

[som-snytt]

I'd forgotten that missing-args was fiddled with a year ago: #4586

[som-snytt]

Note to self, despite ambiguity of assignment, call out the erroneous k=1:

scala> def f(i: Int, j: Int) = i + j
f: (i: Int, j: Int)Int

scala> f(1,2,3)
<console>:13: error: too many arguments (3) for method f: (i: Int, j: Int)Int
       f(1,2,3)
             ^

scala> f(k = 1,i=2,j=3)
<console>:13: error: parameter 'i' is already specified at parameter position 1
Note that 'k' is not a parameter name of the invoked method.
       f(k = 1,i=2,j=3)
                ^
<console>:13: error: too many arguments (3) for method f: (i: Int, j: Int)Int
Note that 'k' is not a parameter name of the invoked method.
       f(k = 1,i=2,j=3)
                    ^

[lrytz]

looks good to me - @janekdb @retronym can you take a look if you also like the new messages? https://github.com/scala/scala/pull/5147/files#diff-159fe691bd78013ef009e40722925f13

[janekdb]

+1

[janekdb]

Exceeds my expectations so have a √LGTM leaving the deciding vote to @retronym.

[som-snytt]

I haven't pushed retronym's idea to push the caret to the bad arg.

[som-snytt]

Rebased. Moves the caret across the goal line.

Also noted illustrious history https://issues.scala-lang.org/browse/SI-3818

[som-snytt]

What's the fifth error?

[som-snytt]

Did it start showing up because the position changed?

[som-snytt]

Yes, in fact under -Ydebug it was:

test/files/neg/protected-constructors.scala:15: error: [ suppressed ] too many arguments for primary constructor Object: ()lang.this.Object
    class Bar3 extends Ding.Foo3("abc")
                            ^

[som-snytt]

@retronym I moved the caret and a suppressed meaningless error is emitted. Is that a deal breaker? I didn't see a quick way to detect "I'd be suppressed at this other position." Lo and behold, a decoupled interface with Reporter is actually inconvenient if I can't interrogate it. Or add API, error(pos, msg)(suppressedByErrorAt = otherpos). Maybe the semantics is, "my underlying cause is at a different pos, at which suppression is computed".

[adriaanm]

The Reporter API is certainly open to evolution (to simplify the process, we could start with an internal subtype that has richer methods than the API used by sbt etc).

[som-snytt]

Positions can be shifted, as in scripts that begin at an offset into the source file.

Analogously, you'd like to say, error(pos withShiftedFocus displayFocus, "too bad"). Then the error-suppressing reporter can check both the shifted focus and the underlying pos.

[adriaanm]

LGTM

[som-snytt]

I'll follow up in future about the spurious error. I didn't see where the inaccessible super ctor results in using nullary ctor. You'd hope the error wouldn't cascade. But I also like that last idea, where an error position (for display) has an underlying unshifted position (for semantics).

[retronym]

But I also like that last idea, where an error position (for display) has an underlying unshifted position (for semantics).

You could model something like that with the current API by issuing another error at the old position with a special message (e.g. empty string) that we could take as meaning it shouldn't be displayed.