Make positions fit for meta #1536
Conversation
Code inspection revealed that it did the wrong thing for annotated trees, looking in the annotation instead of in the argument.
In particular: - get rid of envelope, it's too complicated and hides too many errors - check that everywhere in parsed trees the position range of a parent node contains the position ranges of its children. - check that all non-empty trees coming from parser have positions. The commit contains lots of fixes to make these checks pass. In particular, it changes the scheme how definitions are positioned. Previously the position of a definition was the token range of the name defined by it. That does obviously not work with the parent/children invariant. Now, the position is the whole definition range, with the point at the defined name (care is taken to not count backticks). Namer is changed to still use the token range of defined name as the position of the symbol.
odersky
force-pushed
the
fixes-for-trees
branch
from
dc742a8 to
fb71045
Compare
| private val wild = Ident(nme.WILDCARD) withPos Position(-1) | ||
| private val wildList = List(wild) // OPT This list is shared for performance. | ||
| private def wild = Ident(nme.WILDCARD) | ||
| private def wildList = List(wild) // OPT This list is shared for performance. |
There was a problem hiding this comment.
Not shared anymore since it's a def, it could be removed
Now it's annotated first, annotation second. This is in line with AnnotatedType and in line with the principle that tree arguments should come in the order they are written. The reason why the order was swapped before is historical - Scala2 did it that way.
|
@odersky @liufengyun Could you elaborate on the motivation behind this pull request? |
Check that children of a node have non-overlapping positions and that positions of successive children are monotonically increasing. This holds currently except for 3 exceptions: - Trees coming from Java as the Java parser also does desugarings which copy trees. - Functions coming from wildcard expressions - Interpolated strings We'll see whether we can do something about the latter two.
|
@xeno-by We want parse trees to be as close to source as possible and we want them to have predictable positions. After all positions is all we have to relate to meta trees. |
Arrange its sub-elements so that they appear strictly left to right.
That way, we can check functions for the ordering requirement as well. We only have to remember that the last parameter of a wildcard function does not precede its body (because the parameter is in fact part of the body).
| @@ -505,7 +505,7 @@ object desugar { | |||
| val clsTmpl = cpy.Template(tmpl)(self = clsSelf, body = tmpl.body) | |||
| val cls = TypeDef(clsName, clsTmpl) | |||
| .withMods(mods.toTypeFlags & RetainedModuleClassFlags | ModuleClassCreationFlags) | |||
| Thicket(modul, classDef(cls)) | |||
| Thicket(modul, classDef(cls).withPos(mdef.pos)) | |||
There was a problem hiding this comment.
The pos for Thicket is synthesised via the overloaded def pos, this setting will be overridden.
What about providing a withDerivedPos method for Thicket, instead of overriding def pos?
There was a problem hiding this comment.
No, the withPos applies to the classDef here, not the thicket.
| @@ -63,15 +63,16 @@ object Parsers { | |||
| atPos(Position(start, end, point))(t) | |||
|
|
|||
| def atPos[T <: Positioned](start: Offset, point: Offset)(t: T): T = | |||
| atPos(start, point, in.lastOffset max start)(t) | |||
| if (in.lastOffset > start) atPos(start, point, in.lastOffset)(t) else t | |||
There was a problem hiding this comment.
Throw exceptions instead? Calling atPos without getting a position seems to be an abnormal case.
There was a problem hiding this comment.
No, that's actually a common case. It always happens when the generated tree does not consume any source. In that case we don't set a position and wait for the position to be set by the enclosing node. I'll add a comment to explain.
|
@odersky: If we want to merge the syntax highlighting with the scanner/parser we should give comments a token and position as well - not sure if it should be a part of this PR though... |
|
@felixmulder Let's discuss comment handling separately. |
Fixed finalizers containing branches, and returns inside try-finally. See scala#1536.
Simplifications and checks for consistent positions in parse trees.
Review by @liufengyun.