SI-8125 SI-8130 position fixes for presentation compiler #3337
Conversation
|
Looks like the test I added back then has failed... |
ghost
assigned 
|
@dragos All the failures seem "expected", and the fix should be as simple as updating the tests with a transparent position (instead of an opaque one) for syntehtic EDIT: Actually, while what I said above holds for the test EDIT2: Alright, the reason why the applyDynamic tests fail is that there is another bug (https://issues.scala-lang.org/browse/SI-8130). It's easy to fix, and since the fix in this PR for SI-8125 requires SI-8130 to be fixed as well, I'm going to restructure this PR to contain a fix for both tickets. Hopefully, that's ok. |
Hyperlinking on `d` should resolve to the declaration of `d`, not the `applyDynamic` method in class `D`.
No member is returned by the completion test because the positions assigned to Tree for the `A(` expression are incorrect. In particular, the qualifier `A` should have an opaque range position, while it currently has a transparent one (and that explains why no member is returned in the test).
…icNamed` * Tickets SI-8125 and SI-8130 turn out to be caused by the same underlying issue, i.e., sythetic `apply` Tree where created with the wrong kind of position. The initial fix (look at the first set of changes in Typers.scala) caused a side-effect in the kind of positions assigned to `applyDynamicNamed` synthetic Trees, which changed from transparent to opaque positions. This was fixed by assigning transparent position for synthetic `applyDynamicNamed` Trees (see the second set of changes in Typers.scala). * For test `t8125`, note that `def apply(a: Int): Int` is part of the result returned by the completion test. * For test `t8130`, note that hyperlinking on `d` now is correctly resolved to the declaration of `d`, and not the `applyDynamic` method in class `D`.
|
There is a failing test now in scala-refactoring, I'm looking at it. |
|
PLS REBUILD/pr-scala@82852d9 |
|
(kitty-note-to-self: ignore 32036581) |
|
PLS REBUILD ALL |
|
@retronym this won't work until
When the above two PR are merged, then this PR should finally get a green status. |
|
@dotta although I don't profess to know half of what's going on, this looks very promising for completions =) |
|
@danchia It's only because you are missing context. Once you understand how (Range/Transparent)Position affects the IDE behavior, the changes in this PR makes a lot of sense. If you are interested in the subject, I'm happy to elaborate more in the scala-ide-dev ML ;-) |
|
I need more work on #3358, and because this PR depends on the former, I think it's wise to not rush it. Therefore, you can consider this one as not targeting M8. |
|
@dotta: what's the status of this PR? |
|
@gkossakowski It needs more work and unfortunately I don't have time to allocate on this - work stealing is of course welcomed. |
|
Okay, we'll close the PR for now. Let's push again for 2.11.1! |
review by @retronym
@danchia This PR should make you happy ;-)