Skip to content

Issue/semantic highlight type params #179

Merged
merged 11 commits into from Aug 13, 2012

3 participants

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

This PR adds a lot of semantic highlighting support for type params.

I'm not completely satisfied with the result because the presentation compiler doesn't store enough information in the AST to resolve all problems. Sometimes I had to analyze the AST by myself and add a some additional test cases to be sure that I did not broke anything. However, at the end, with each commit I could improve semantic highlighting.

  • compound types are not completely highlighted (only the type params of parameterized types)
  • nothing in structural types is highlighted but the method names are
  • view bounds were quite difficult but I hope the current solution (+ test cases) is enough.

Please review about

  • commit messages. Are they enough?
  • ignored test cases. Is it good to have them separated from the ones working?
  • my manually AST traverses. Maybe I have overseen a simple solution.
@dotta dotta and 1 other commented on an outdated diff Aug 12, 2012
...mantichighlighting/classifier/TypeParameterTest.scala
@@ -70,4 +70,181 @@ class TypeParameterTest extends AbstractSymbolClassifierTest {
""",
Map("TPARAM" -> TypeParameter, "T" -> Type))
}
+
+ @Test
+ def partial_compound_type_param() {
+ checkSymbolClassification("""
+ trait X {
+ def xs[TypeParam]: Seq[TypeParam] with collection.IterableLike[TypeParam, TypeParam]
+ }
+ """, """
+ trait X {
+ def xs[$TPARAM $]: Seq[$TPARAM $] with collection.IterableLike[$TPARAM $, $TPARAM $]
+ }
+ """,
+ Map("TPARAM" -> TypeParameter, "T" -> Type))
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

I think you don't need "T" -> Type (wild copy/paste? :-))

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

I will remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta commented on an outdated diff Aug 12, 2012
...mantichighlighting/classifier/TypeParameterTest.scala
+ def xs[$TPARAM $]: Seq[$TPARAM $] with collection.IterableLike[$TPARAM $, $TPARAM $]
+ }
+ """,
+ Map("TPARAM" -> TypeParameter, "T" -> Type))
+ }
+
+ @Test
+ @Ignore("does not work until presentation compiler stores more information in the AST")
+ def full_compound_type_param() {
+ checkSymbolClassification("""
+ trait X {
+ def xs[TypeParam]: Seq[TypeParam] with collection.IterableLike[TypeParam, TypeParam]
+ }
+ """, """
+ trait X {
+ def xs[$TPARAM $]: $T$[$TPARAM $] with collection.$Trait $[$TPARAM $, $TPARAM $]
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

This collection.$Trait looks suspicious. But I may be missing something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta and 1 other commented on an outdated diff Aug 12, 2012
...mantichighlighting/classifier/TypeParameterTest.scala
+ Map("TPARAM" -> TypeParameter, "T" -> Type))
+ }
+
+ @Test
+ @Ignore("does not work until presentation compiler stores more information in the AST")
+ def full_compound_type_param() {
+ checkSymbolClassification("""
+ trait X {
+ def xs[TypeParam]: Seq[TypeParam] with collection.IterableLike[TypeParam, TypeParam]
+ }
+ """, """
+ trait X {
+ def xs[$TPARAM $]: $T$[$TPARAM $] with collection.$Trait $[$TPARAM $, $TPARAM $]
+ }
+ """,
+ Map("TPARAM" -> TypeParameter, "T" -> Type, "TRAIT" -> Trait))
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

Do you need "T" -> Type, "TRAIT" -> Trait?

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

Yes, typo error.

@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

Ok, this also explains my above comment ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta commented on the diff Aug 12, 2012
...mantichighlighting/classifier/TypeParameterTest.scala
+ }
+
+ @Test
+ def tuple_literal_type_param() {
+ checkSymbolClassification("""
+ trait X {
+ def f: (Int, String)
+ def g: Tuple2[Int, String]
+ }
+ """, """
+ trait X {
+ def f: ($C$, $TYPE$)
+ def g: $CC $[$C$, $TYPE$]
+ }
+ """,
+ Map("TYPE" -> Type, "C" -> Class, "CC" -> CaseClass))
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

"CC" -> CaseClass not needed.

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

def g needs CC. Have you overseen it or do you mean that it is unnecessary to check the tuple class?

@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

Ooops. Sorry, my mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta commented on an outdated diff Aug 12, 2012
...ipse/semantichighlighting/classifier/SafeSymbol.scala
@@ -52,7 +52,7 @@ trait SafeSymbol extends CompilerAccess with PimpedTrees {
// if the original tree did not find anything, we need to call
// symbol, which may trigger type checking of the underlying tree, so we
// wrap it in 'ask'
- if (originalSym.isEmpty) {
+ if (originalSym.isEmpty && tpeTree.pos.isRange) {
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

Why the check for range position? Could you add a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta and 1 other commented on an outdated diff Aug 12, 2012
...ipse/semantichighlighting/classifier/SafeSymbol.scala
}
+
+ private def isViewBound(args: List[Tree]): Boolean =
+ args.size == 2
+
+ private def isProbableTypeBound(name: Name): Boolean =
+ name.startsWith("evidence$")
+
+ private def isStructuralType(ts: List[Tree]): Boolean =
+ ts.size == 1
+
+ private def isTupleOrFunctionLiteral(tpt: Tree, qualifier: Tree, name: Name): Boolean = (
+ qualifier.nameString == "scala"
+ && (name.toString.startsWith("Function") || name.toString.startsWith("Tuple"))
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

Why not using a regex, so that the naming match is precise. WDYT?

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

Yes, good idea. I didn't think to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta and 1 other commented on an outdated diff Aug 12, 2012
...ipse/semantichighlighting/classifier/SafeSymbol.scala
+
+ case tpe @ SelectFromTypeTree(qualifier, _) =>
+ tpe.symbol -> tpe.namePosition :: safeSymbol(qualifier)
+
+ case CompoundTypeTree(Template(parents, _, body)) =>
+ (if (isStructuralType(parents)) body else parents).flatMap(safeSymbol)
+
+ case TypeBoundsTree(lo, hi) =>
+ List(lo, hi).flatMap(safeSymbol)
+
+ case ValDef(_, name, tpt: TypeTree, _) if isProbableTypeBound(name) =>
+ tpt.original match {
+ case AppliedTypeTree(_, args) if isViewBound(args) =>
+ safeSymbol(args(1))
+ case AppliedTypeTree(tpe, args) if isContextBound(args) =>
+ List(tpe.symbol -> tpe.namePosition)
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

tpt.symbol can trigger side-effects (i.e., lazy typechecking), you should wrap this in a askOption, i.e., replace List(tpe.symbol -> tpe.namePosition) with global.askOption(() => (tpe.symbol, tpe.namePosition)).toList

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

Ok, good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta commented on an outdated diff Aug 12, 2012
...ipse/semantichighlighting/classifier/SafeSymbol.scala
case AppliedTypeTree(tpe, args) =>
- tpe.symbol -> tpe.namePosition :: args.flatMap(safeSymbol)
+ (tpe :: args).flatMap(safeSymbol)
+
+ case tpe @ SelectFromTypeTree(qualifier, _) =>
+ tpe.symbol -> tpe.namePosition :: safeSymbol(qualifier)
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

tpt.symbol can trigger side-effects (i.e., lazy typechecking), you should wrap this in a askOption, i.e., replace tpe.symbol -> tpe.namePosition :: safeSymbol(qualifier) with global.askOption(() => (tpe.symbol, tpe.namePosition)).toList ::: safeSymbol(qualifier)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta and 1 other commented on an outdated diff Aug 12, 2012
...ipse/semantichighlighting/classifier/SafeSymbol.scala
}
+
+ private def isViewBound(args: List[Tree]): Boolean =
+ args.size == 2
+
+ private def isProbableTypeBound(name: Name): Boolean =
+ name.startsWith("evidence$")
@dotta
Eclipse Scala IDE member
dotta added a note Aug 12, 2012

If it works, it's better to use nme.EVIDENCE_PARAM_PREFIX. Though, it may not compile with Scala 2.9. (you need to test it :-)). If the name constant is not available in Scala 2.9, keep your code, but add a note so that we remember to get back to this after we drop support for Scala 2.9.

@sschaef
Eclipse Scala IDE member
sschaef added a note Aug 12, 2012

I executed the 2.9 test suite and all tests succeeded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

WOW! This is such an awesome PR, Simon. Great job!

Please review about

* commit messages. Are they enough?

Are there any opened ticket for the issues you have fixed? If yes, remember to have a Fix #<number> in your commit message.

* ignored test cases. Is it good to have them separated from the ones working?

I'm ok with this, however you didn't separate them in this PR, did you?

* my manually AST traverses. Maybe I have overseen a simple solution.

Looks good to me, but I'm not the compiler guru (Iulian is, but he's currently on holiday ;-) \cc @dragos)

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

Oh, there is one more thing I should ask you to do: can you rebase your PR on the latest master? The reason is that 2.10 build should then succeed.

  • Update: Actually, you may not need to do this, the PR validator was not in a clean state. Let me try to re-build
@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

And I noticed another thing: In this commit message you say:

The presentation compiler doesn't store enough information in the AST in order to exactly determine if a view bound is found

The one to blame is not the presentation compiler, but the actual compiler itself ;-) (you may want to update the message)

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

PLS REBUILD ALL

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

I didn't push the amends (I did only amends no new commits) yet. Can I do this normally with git push origin issue/semantic-highlight-ype-params or will this break some comments above (pointing to line numbers)?

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

No luck, your branch is based on a old master that does no longer compile because of binary incompatibilities in sbt (which we use as the IDE internal project's builder). You'll have to rebase...

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

The one to blame is not the presentation compiler, but the actual compiler itself ;-) (you may want to update the message)

I don't know where one ends and the other begins. ;) I thought the typer phase (the phase I working with in semantic highlighting?) is only done by the presentation compiler.

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

I didn't push the amends (I did only amends no new commits) yet. Can I do this normally with git push origin issue/semantic-highlight-ype-params or will this break some comments above (pointing to line numbers)?

When you amend a commit, you are changing the history of your branch, and hence pushing will fail (you can always safely try out git commands by appending --dry-run). What you can do is a force push (i.e., git push -f origin issue/semantic-highlight-type-params), and this PR will be automatically updated. If you do so, make sure to go through all comments first, I'm not sure what will happen to the comments once you force push the branch (they are not deleted, but they can get into some weird unsync state and no longer be linked to a code line).

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

No luck, your branch is based on a old master that does no longer compile because of binary incompatibilities in sbt (which we use as the IDE internal project's builder). You'll have to rebase...

How to do this? A normal

git checkout master
git fetch upstream
git merge upstream/master
git checkout issue/semantic-highlighting-type-params
git rebase master

did not work (all up to date).

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

Are there any opened ticket for the issues you have fixed? If yes, remember to have a Fix # in your commit message.

No, there aren't. I was to lazy to create one. Is it useful to do it?

I'm ok with this, however you didn't separate them in this PR, did you?

With separating I meant separating them into an own test case.

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012

I don't know where one ends and the other begins. ;) I thought the typer phase (the phase I working with in semantic highlighting?) is only done by the presentation compiler.

That's a good question :-)

The presentation compiler executes only a subset of phases compared to the standard scala compiler, but these phases are the very same. The only real difference between the two is that a presentation compiler pass stops at the typer phase, instead of going until bytecode generation.

The main responsability of the presentation compiler is providing an interface for communicating with the scala compiler.

@dotta
Eclipse Scala IDE member
dotta commented Aug 12, 2012
  • About the rebasing, your commands looks good to me. Since it failed, try to go with an interactive rebase, i.e., git rebase -i master. You may need to solve some conflict, but I'd expect this to be easy for this PR.

  • It would be good to create at least one ticket with the list of functionalities you have implemented. This helps us creating a meaningful changelog when we release (so that people can know what has improved in the release)

With separating I meant separating them into an own test case.

What's a test case, a method? a class? Sorry, I'm having troubles following this. But if you want to split working and ignored tests in two separated test classes, I'm fine with this, as long as you add both classes in the TestSuite

sschaef added some commits Aug 12, 2012
@sschaef sschaef Add semantic highlighting for view bounds
The compiler doesn't store enough information in the AST in order to
exactly determine if a view bound is found. Thus, it is necessary to
analyze the members of the tree types manually which is error-prone.
To be sure that no bugs are caused, additional test cases were introduced.
9c00a68
@sschaef sschaef Add test case for higher kinded types to semantic highlighting test s…
…uite
3327343
@sschaef sschaef Add semantic highlighting for existential types 00e131a
@sschaef sschaef Add semantic highlighting for type projections and path dependent types cee80b8
@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

PLS REBUILD ALL

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 12, 2012

git rebase -i master works fine, thanks.

What's a test case, a method?

Yes, I meant a method. Sry for the wrong word. I'll let it as it is, it is ok.

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

Yes, I meant a method. Sry for the wrong word. I'll let it as it is, it is ok.

Perfect. So, the only thing that is missing is a ticket that lists all your changes.

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

I also wonder if there isn't a more precise way to check isViewBound, isStructuralType, isContextBound and similar. I'll ask the EPFL folks and get back to you during the day.

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

I also wonder if there isn't a more precise way to check isViewBound, isStructuralType, isContextBound and similar. I'll ask the EPFL folks and get back to you during the day.

I checked and apparently there is no better way, so it's all good. As soon as you create a ticket for this, we can merge this PR.

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 13, 2012

As soon as you create a ticket for this, we can merge this PR.

Done: http://www.assembla.com/spaces/scala-ide/tickets/1001204

Does it provide enough information or should I add more (links to PR for example)?

@dotta dotta merged commit 3f934f8 into scala-ide:master Aug 13, 2012
@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

Fantastic! I have merged and updated the ticket. Keep on rockin' :-)

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

I see now we have quite a few open tickets for Semantich Highlighting: http://scala-ide-portfolio.assembla.com/spaces/ae55a-oWSr36hpeJe5avMc/tickets/report/u362933

Are you sure that none of them can be closed after your PR? (or, if you are having fun with semantic highlighting, you could have a look at them ;-))

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 13, 2012

I already looked at these tickets, but this PR doesn't solve any one them. Half of the tickets there are submitted by me and I spent already some time on them, but again it was the Scala compiler which doesn't store enough information in the AST. Thus, I could not solve the problems. :(

I will going on working on semantic highlighting. Hopefully I will find a ticket which can be solved...

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

I already looked at these tickets, but this PR doesn't solve any one them. Half of the tickets there are submitted by me and I spent already some time on them, but again it was the Scala compiler which doesn't store enough information in the AST. Thus, I could not solve the problems. :(

Eh eh eh, I did not pay attention to the fact that it was you that filed the tickets :-) And you are absolutely right, the compiler AST doesn't always provide enough information, and that's why semantic highlight also use the scalariform AST (did you noticed it?). Maybe with the scalariform AST you can find the missing information (it's something to investigate if you have time and will).

I will going on working on semantic highlighting. Hopefully I will find a ticket which can be solved...

I know we're nowhere near XMas, but here is my wish list :-)

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 13, 2012

What shall I do with my local branch after the merge? Shall I delete it?

@sschaef
Eclipse Scala IDE member
sschaef commented Aug 13, 2012

and that's why semantic highlight also use the scalariform AST (did you noticed it?).

Yeah, I noticed it, but didn't understand why it is there ;)

Maybe with the scalariform AST you can find the missing information (it's something to investigate if you have time and will).

Let's see what I can do.

@dotta
Eclipse Scala IDE member
dotta commented Aug 13, 2012

What shall I do with my local branch after the merge? Shall I delete it?

There is really no reason to keep them around ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.