Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed positions in de-aliased special symbols.. #1251

Merged
merged 1 commit into from Sep 4, 2012

Conversation

Projects
None yet
4 participants
Contributor

dragos commented Sep 4, 2012

..and for automatically adde.d apply methods.

Fixed #5064, thanks to @paulp who showed the right direction (and how to test it).

review by @paulp.

Fixed positions in de-aliased special symbols and for automatically a…
…dded `apply` methods.


Fixed #5064, thanks to @paulp who showed the right direction (and how to test it).

@paulp paulp commented on the diff Sep 4, 2012

src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -579,7 +579,7 @@ trait Typers extends Modes with Adaptations with Tags {
// to notice exhaustiveness and to generate good code when
// List extractors are mixed with :: patterns. See Test5 in lists.scala.
def dealias(sym: Symbol) =
- (atPos(tree.pos) {gen.mkAttributedRef(sym)}, sym.owner.thisType)
+ (atPos(tree.pos.makeTransparent) {gen.mkAttributedRef(sym)} setPos tree.pos, sym.owner.thisType)
@paulp

paulp Sep 4, 2012

Contributor

This code again! Will this hack never tire of creating new issues.

Contributor

paulp commented Sep 4, 2012

LGTM.

Contributor

dragos commented Sep 4, 2012

(the build failure is spurious, due to typesafe artifactory being down).

paulp added a commit that referenced this pull request Sep 4, 2012

Merge pull request #1251 from dragos/issue/fix-5064
Fixed positions in de-aliased special symbols..

@paulp paulp merged commit 9556dfb into scala:2.10.x Sep 4, 2012

@misto misto commented on the diff Sep 11, 2012

src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1057,7 +1057,7 @@ trait Typers extends Modes with Adaptations with Tags {
case other =>
other
}
- typed(atPos(tree.pos)(Select(qual, nme.apply)), mode, pt)
+ typed(atPos(tree.pos)(Select(qual setPos tree.pos.makeTransparent, nme.apply)), mode, pt)
@misto

misto Sep 11, 2012

Why is qual getting the transparent position? Shouldn't it be the Select, like Select(qual, nme.apply) setPos tree.pos.makeTransparent

@dragos

dragos Sep 11, 2012

Contributor

It makes sense, but if the apply was transparent there would be no way to get to that tree.

With the current solution, one can locate the Apply(qual.. tree, and it has access to both the qualifier and selection. This allowed the IDE to offer hyperlinking to both List.apply and List on that position.

If we make the apply transparent, one will always retrieve the qualifier. Since trees have no way to get to their parent, there would be no way to hyperlink or get to the apply node anymore.

Does this solution make your life much harder?

@misto

misto Sep 11, 2012

Ok, I see that this is convenient for the IDE. If it's just for apply, then it should be easy to have a workaround in the refactoring library. It just doesn't feel like a good solution, abusing TransparentPositions like this :)

So, what about unapply calls? Shouldn't they get the same treatment?

@dragos

dragos Sep 11, 2012

Contributor

I'm open to better solutions, or I can remove this change if you prefer.

I think unapplies are translated after type-checking (there's an UnApply node)..

@misto

misto Sep 11, 2012

I just checked what happens to unapply. The UnApply tree in a pattern has an Apply with an Extractor.unapply Select. Here the Extractor Ident simply has the same range position as the Extractor.unapply Select. To me that also doesn't look right..

I'm not sure what to do, in my understanding of positions, both apply and unapply should be transparent positions and their underlying Ident or Select trees should have a range position. But if that prevents the IDE from hyperlinking directly to the definition of apply, then maybe we should just leave this as it is now.

dragos added a commit to dragos/scala-ide that referenced this pull request Sep 11, 2012

Fix hyperlinks to `classOf` and other instances that were not treated…
… correctly.

Hyperlinking to `List(1,..)` returns two results (`List.apply` and `List`). Same
for any call to `apply`.

Also refactored the oldest test in the project, simpleHyperlinks. Now it 
uses the same infrastructure as the other hyperlink tests.

Added support for hyperlinks that return more than one result in the test framework.

Depends on scala/scala#1251 to be merged and back ported.

Fixed #1001238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment