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

Prefer focus for missing arg position #15644

Closed
wants to merge 3 commits into from

Conversation

som-snytt
Copy link
Contributor

The focus puts the caret at the bad application, instead of hanging in space where an explicit argument might have gone in an alternate reality.

The two benefits are pointing at an existing thing that is wrong and also shifting the messaging left.

Sample dramatic diff:

-- Error: tests/neg/i14025.scala:1:16 ----------------------------------------------------------------------------------
1 |val foo = summon[deriving.Mirror.Product { type MirroredType = [X] =>> [Y] =>> (X, Y) }] // error
  |                                                                                        ^
  |                ^

Further future enhancements could include the hypothetical args printed in Scala 2 messages, perhaps under -explain:

f(x, y, z)
^
The missing context arguments were expected as follows:
f(e1, e2)(x, y, z)
 ^^^^^^^^

Fixes #15630

@dwijnand
Copy link
Member

dwijnand commented Jul 11, 2022

I almost feel like start is better than focus? Ie rather than the change being:

 2 |  val x = implicitly[List[Boolean]] // error
-  |                                   ^
+  |                    ^
   |
 8 |  val value3 = M.pure("ola") // error
-  |                            ^
+  |                     ^

make it:

 2 |  val x = implicitly[List[Boolean]] // error
-  |                                   ^
+  |          ^
   |
 8 |  val value3 = M.pure("ola") // error
-  |                            ^
+  |                 ^

WDYT?

@som-snytt
Copy link
Contributor Author

som-snytt commented Jul 11, 2022

When you put it that way, I like my suggestion for future enhancement, just show me the signature with the holes.

I see there is already similar formatting:

   |I found:
   |
   |    FooT.OrderFFooA[F, A](FooT.OrderFFooA[F, A](/* missing */summon[Order[F[Foo[A]]]]))

@som-snytt som-snytt force-pushed the issue/15630-caret-context-arg branch from ca00eb1 to 7a5e6a4 Compare July 11, 2022 19:33
@som-snytt
Copy link
Contributor Author

I really feel like I'm not so stupid that it should take more than one or two rounds of tests to see which tests failed. I will endeavour [typelevel spelling] to figure out why it can't just summarize at the end and eliminate the scrolling.

@som-snytt
Copy link
Contributor Author

Excerpt from my journal during this expedition:

Day 37, I moved the caret to the left, then all the tests expecting // error on a particular line number fail if the caret shifted to the previous line. Moreover, Pixie the test rig is coy about showing me her diff. I must be going mad.

@som-snytt som-snytt force-pushed the issue/15630-caret-context-arg branch from 7a5e6a4 to 9a10c98 Compare July 13, 2022 03:24
@som-snytt som-snytt force-pushed the issue/15630-caret-context-arg branch from 9a10c98 to 9fb3ada Compare July 13, 2022 04:40
@som-snytt som-snytt marked this pull request as ready for review July 13, 2022 17:48
@som-snytt
Copy link
Contributor Author

@dwijnand I think my previous comment, that "an improved show-me-the-holes message" is ideal, but I maintain that this delta is an improvement. I'm still very bothered by carets pointing at whitespace.

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.

Invalid caret position for missing given argument error messages
2 participants