Line content from diagnostic classes if available #2108
Conversation
|
Hi @fkorotkov, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
|
Can one of the admins verify this patch? |
|
@typesafehub-validator signed. |
| case Some(content) => content.equalsIgnoreCase(p.position.lineContent()) | ||
| case _ => true | ||
| } | ||
| def lineNumberCheck = p.position.line.isDefined && (p.position.line.get == lineno) |
There was a problem hiding this comment.
Option.get should be avoided:
def lineNumberCheck = p.position.line.exists(_ == lineno)
There was a problem hiding this comment.
Unfortunately it is xsbti.Maybe and there is no exists method
There was a problem hiding this comment.
AH, right. Maybe is the limited interface for java-interop/classloader boundary fun. Sorry!
|
Thanks for the pull request. Looks like it won't merge cleanly, so it needs a rebase. I've also added some comments. |
| def getDiagnosticLine: Option[String] = | ||
| try { | ||
| // See com.sun.tools.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper | ||
| val diagnostic = d.getClass.getField("d").get(d) |
There was a problem hiding this comment.
I'm really nervous casting directly to com.sun.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper internals.
IF we do this, I think the alternative mechanism (that was here before) should be a fallback in the event these classes do not exist. (i.e. alternative JDKs).
|
Thanks for the PR. A few things to clean up, but having the option to go right after the JDK classes will be nice. As I said, I'd like to also use the previous mechanism as a fallback before finally not having any diagnostic info. |
|
Thank you guys for the feedback. Addressed in https://github.com/fkorotkov/sbt/commit/c73f513016a8c2be94decfda8ea83b6e1b95601d. @jsuereth I do fall back to the previous behavior. See https://github.com/sbt/sbt/pull/2108/files#diff-9b267fe75b6831da3dca25ceb91082e9R90 |
|
@fkorotkov Yeah, the previous behavior was that (casting to JavaObject). I know that worked for SOME files. We know your reflection will work for Oracle/OpenJDK (I think). When it comes to these kinds of behaviors that aren't fully specified, we have to "hope for the best" and leave hooks for people who care about the non-standard JDKs to patch in. If we have |
| case _ => "" | ||
| } | ||
|
|
||
| getDiagnosticLine.getOrElse(getExpression) |
|
Thanks much @fkorotkov ! Really glad to have better tooling support on the java side of the house here! |
Line content from diagnostic classes if available
|
Thank you guys! I initially though to use source.getCharContent and split it in lines. But then I check how javac works and went that way. But now as you mentioned other JDK I think it might be a better solution. Anyway source.getCharContent is already used in the default behavior. |
Problem
While using local java compiler line content in errors is wrong. Only an expression is shown and not a whole line of code.
Expectation
Get a proper line content.
Solution
When you run javac from command line com.sun.tools.javac.util.AbstractDiagnosticFormatter#formatSourceLine is used to format an output. There diagnostic classes are used to get line content. Made a change to try to use diagnostic classes if they are available.