Skip to content

Include span of bad args if multiline#25699

Merged
mbovel merged 1 commit intoscala:mainfrom
som-snytt:issue/9434-error-src-ctx
Apr 10, 2026
Merged

Include span of bad args if multiline#25699
mbovel merged 1 commit intoscala:mainfrom
som-snytt:issue/9434-error-src-ctx

Conversation

@som-snytt
Copy link
Copy Markdown
Contributor

@som-snytt som-snytt commented Apr 2, 2026

Fixes #9434

The message position is tweaked only when the args are on a different line.

Then the source context includes the args; but only one caret for the point.

It would be nicer to request more context without changing the diagnostic position.

@som-snytt som-snytt marked this pull request as draft April 3, 2026 14:50
@som-snytt som-snytt force-pushed the issue/9434-error-src-ctx branch from 01209e4 to a5c5232 Compare April 4, 2026 04:29
@som-snytt som-snytt changed the title Show the params it doesn't take Include span of bad args if multiline Apr 4, 2026
@som-snytt som-snytt marked this pull request as ready for review April 4, 2026 04:36
@som-snytt som-snytt force-pushed the issue/9434-error-src-ctx branch 2 times, most recently from 86ef31e to c663284 Compare April 4, 2026 16:38
@Gedochao Gedochao requested review from mbovel and warcholjakub April 8, 2026 07:53
if pt.args.isEmpty then tree.srcPos
else
val union = tree.sourcePos.withSpan:
tree.srcPos.span.union(pt.args.last.srcPos.span)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if tree.sourcePos is remapped (e.g. code has a magic header), while the union uses raw srcPos spans? Could that shift reported locations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll look at how that is done. On Scala 2, the "underlying" position was a function of the source. Otherwise, how could one do any position calculation? However, I endorse my comment about using API to request more context instead of touching position.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previously, shifting for funky headers was positionInUltimateSource, which may still be used in two places.

sourcePos.withSpan is the same as tree.source.atSpan or

val union = tree.sourcePos.copy(span = tree.srcPos.span.union(pt.args.last.srcPos.span))

Here we don't have a Positioned, we just want a SrcPos for reporting. Normally, reporting creates a Diagnostic and its SourcePosition together.

I won't push it further here, but it would be nice if reporting had a channel for extra pos info like inlined outers (currently a field in SourcePosition but really belongs to the diagnostic) and for this kludge a pos to say "rendering context should include this line".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. To be sure I tested it, and it doesn't seem to affect the case I mentioned.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know what IDEs do with multiline squiggles, however. Also, one could not do this trick and also emit a rewrite suggestion.

@som-snytt som-snytt force-pushed the issue/9434-error-src-ctx branch from c663284 to 374ac9b Compare April 10, 2026 01:26
@som-snytt
Copy link
Copy Markdown
Contributor Author

Just a rebase and a random NPE. We ought not to normalize that.

[info] Test dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS started
Test 'tests/run/i22888.scala' failed with output:                               
java.lang.NullPointerException: Cannot invoke "scala.concurrent.Future.onComplete(scala.Function1, scala.concurrent.ExecutionContext)" because "that" is null
	at scala.concurrent.impl.Promise$DefaultPromise.zipWith(Promise.scala:153)
	at scala.concurrent.Future$.traverse$$anonfun$1(Future.scala:884)
	at scala.collection.IterableOnceOps.foldLeft(IterableOnce.scala:740)
	at scala.collection.IterableOnceOps.foldLeft$(IterableOnce.scala:336)
	at scala.collection.AbstractIterator.foldLeft(Iterator.scala:1325)
	at scala.concurrent.Future$.traverse(Future.scala:884)
	at org.scalajs.linker.standard.StandardIRFileCache$CacheImpl.cached(StandardIRFileCache.scala:67)
	at dotty.tools.dotc.ScalaJSLink$.$anonfun$3(ScalaJSLink.scala:52)
	at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:489)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1395)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)

Error:  Test dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS failed: java.lang.AssertionError: Run test failed, but should not, reasons:
Error:  encountered 1 test failure(s):
Error:    - generic failure (see test output), took 428.099 sec
Error:      at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkPass(ParallelTesting.scala:1299)
Error:      at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkRuns(ParallelTesting.scala:1259)
Error:      at dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS(ScalaJSCompilationTests.scala:33)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
Error:      at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Error:      at java.lang.reflect.Method.invoke(Method.java:569)
Error:      ...
[info] Test dotty.tools.dotc.ScalaJSCompilationTests.negScalaJS started

================================================================================
Test Report
================================================================================

1 suites passed, 1 failed, 2 total
    tests/run/i22888.scala failed


[info] Test run dotty.tools.dotc.ScalaJSCompilationTests finished: 1 failed, 0 ignored, 2 total, 431.226s
Error:  Failed: Total 2, Failed 1, Errors 0, Passed 1
Error:  Failed tests:
Error:  	dotty.tools.dotc.ScalaJSCompilationTests
Error:  (sjsCompilerTests / Test / test) sbt.TestsFailedException: Tests unsuccessful
Error:  Total time: 818 s (0:13:38.0), completed Apr 10, 2026, 2:30:22 AM
Error: Process completed with exit code 1.

Copy link
Copy Markdown
Member

@warcholjakub warcholjakub left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

This PR enhance error messages about extra parameters for multi-line method calls by also showing the extra arguments that are on separate lines in the error message.

For example, before we would show:

-- [E050] Type Error: tests/neg/19087.scala:13:22 ----------------------------------------------------------------------
13 |        foo(state.copy(x = 5): // Missing ")" // error: method copy in class State does not take more parameters
   |            ^^^^^^^^^^^^^^^^^
   |            method copy in class State does not take more parameters
   |

And after:

-- [E050] Type Error: tests/neg/19087.scala:13:22 ----------------------------------------------------------------------
13 |        foo(state.copy(x = 5): // Missing ")" // error: method copy in class State does not take more parameters
   |            ^
   |            method copy in class State does not take more parameters
   |
14 |          println("a")
   |

Note the new line 14 | println("a").

It looks good to me!

@mbovel mbovel merged commit 1610a73 into scala:main Apr 10, 2026
106 of 107 checks passed
@som-snytt som-snytt deleted the issue/9434-error-src-ctx branch April 10, 2026 15:32
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.

Indentation causes incorrect parsing and/or unhelpful errors

3 participants