JS: Update DiagnosticLocation call to gracefully handle invalid locations
#12895
Conversation
henrymercer
added
the
depends on internal PR
| @Override | ||
| public int compare(File f1, File f2) { |
There was a problem hiding this comment.
This indentation looks weird?
There was a problem hiding this comment.
Thanks, fixed. Out of interest, how do you tend to edit these files? There doesn't seem to be a formatter configured in Eclipse.
| if (diagLoc.isPresent()) { | ||
| writeDiagnostics(msg, JSDiagnosticKind.PARSE_ERROR, diagLoc.get()); | ||
| } else { | ||
| msg += "\n\nRelated file: " + file; |
There was a problem hiding this comment.
I'm not sure we should "leak" the folder hierarchy of the machine on which the analysis was run into the diagnostic message.
We should probably include a DiagnosticLocation with just the file path if the build() method fails due to invalid position information.
There was a problem hiding this comment.
I'm not sure we should "leak" the folder hierarchy of the machine on which the analysis was run into the diagnostic message.
That seems reasonable, particularly for the status page. Done.
We should probably include a
DiagnosticLocationwith just the file path if thebuild()method fails due to invalid position information.
We're just checking startLine <= endLine, so my understanding is we should never fail here due to invalid position information.
henrymercer
force-pushed
the
henrymercer/diagnostics-verify-one-based
branch
from
c3f98c4 to
f91da38
Compare
henrymercer
force-pushed
the
henrymercer/diagnostics-verify-one-based
branch
from
f91da38 to
927522c
Compare
|
@aibaars @erik-krogh Would you mind taking another look? Thanks! |
| .build(); | ||
| writeDiagnostics(msg, JSDiagnosticKind.PARSE_ERROR, diagLoc); | ||
| .build() | ||
| .getOk(); |
There was a problem hiding this comment.
ql/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java:1252: error: cannot find symbol
.getOk();
^
symbol: method getOk()
location: class DiagnosticLocation
There was a problem hiding this comment.
This is due to us changing the DiagnosticLocation API in the corresponding internal PR (backlinked above). The tests are passing there.
There was a problem hiding this comment.
I guess this PR depends on another PR.
This PR updates the JS autobuilder's use of the
DiagnosticLocationclass to take advantage of an internal change that more gracefully handles invalid locations.