Set proper position for ValDefs generated from tuples #14513
Conversation
|
Relatedly, multi-assignment is also weird. I was looking at a position ticket during pandemic fog, so I don't remember details. I closed the original ticket as working under |
| if (mods.is(Lazy)) derivedDefDef(original, named, tpt, selector(n), mods &~ Lazy) | ||
| else derivedValDef(original, named, tpt, selector(n), mods) |
There was a problem hiding this comment.
The point of derivedValDef/derivedDefDef is that they set their span based on the original tree node, here you're going to overwrite that using withSpan, so you don't need these methods and could just inline them (derivedValDef(...) becomes valDef(ValDef(...).withMods(mods)), derivedDefDef(...) becomes DefDef(...).withMods(mods))
There was a problem hiding this comment.
Thanks! I inlined it all!
I added test case for it and it seems fine. I will also check later in Metals Edit: |
| @@ -93,12 +93,6 @@ class WorksheetTest { | |||
| ((m1 to m2), "val res0: String = odd")) | |||
| } | |||
|
|
|||
| @Test def patternMatching1: Unit = { | |||
There was a problem hiding this comment.
Removed the test because it was failing and since worksheets deprecated. I think the issue is more in the test than code itself.
| @@ -20,7 +20,7 @@ object Test { | |||
| // good syntax, bad semantics, detected by typer | |||
| //gowild.scala:14: error: star patterns must correspond with varargs parameters | |||
| val K(x @ _*) = k | |||
| val K(ns @ _*, x) = k // error: pattern expected | |||
| val K(ns @ _*, xx) = k // error: pattern expected | |||
There was a problem hiding this comment.
This is an interesting case. x started showing and error about being duplicate, which is true. I changed the condition to test the actual issue.
I wonder if I should add another case for the duplicate name?
There was a problem hiding this comment.
I added a test, the error seems legit, though I have no idea why it popped up now.
There was a problem hiding this comment.
I have no idea why it popped up now.
When multiple errors have the same position, we normally only display the first one.
Previously, the span for ValDefs generated for tuples would encompas the entire expression, which led to difficulties identifying the exact path to the current position. Now, we set the span to be the same as the name underneath. Not sure if this is a proper solution, since normally ValDefs ecompans the entire span, but in this case it makes 2 different ValDef have the same span. An alternative solution would be to find the one with point nearer to the current position. This popped up within the Nightly tests we do in Metals.
Previously, the span for ValDefs generated for tuples would encompas the entire expression, which led to difficulties identifying the exact path to the current position. Now, we set the span to be the same as the name underneath.
Not sure if this is a proper solution, since normally ValDefs ecompans the entire span, but in this case it makes 2 different ValDef have the same span. An alternative solution would be to find the one with point nearer to the current position.
This popped up within the Nightly tests we do in Metals.