Rust: Add Missing Elements query #17465
Conversation
geoffw0
added
no-change-note-required
|
Hmm, the locations need cleaning up here... |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
OK, we get different results on CI to my run locally. I think this will be a continuous source of problems unless we either (1) restrict results to inside the source location or (2) remove or constrain the test. (1) seems reasonable but it does mean we won't have visibility on problems extracting the standard library. I'll have another think about it on Monday. Opinions welcome. |
My other PR no longer extracts files in the standard library or dependencies so that problem should be solved. I added a flag to enable the extraction of libraries but by default we no longer do. Extracting the standard library is too brittle because results depend on whichever version of rust is installed and most likely the platform on which it runs also causes differences. |
|
@geoffw0 @redsun82 This PR has a compilation error because Location getLocation(Unimplemented node) {
result = node.(Locatable).getLocation() or
result instanceof UnknownLocation and not node instanceof Locatable}
}to this PR. Not sure what is best, I'm fine with either approach. |
Does that mean we'll have difficulty assigning targets for calls to standard library functions? Or are we extracting some stuff in the standard libraries still? |
| c = strictcount(Unimplemented n | cleanLocationString(getUnimplementedLocation(n)) = location) and | ||
| c = | ||
| strictcount(Unimplemented n | | ||
| exists(getUnimplementedLocation(n).getFile().getRelativePath()) and |
There was a problem hiding this comment.
This excludes elements from user code for which the extractor did not know a location.
There was a problem hiding this comment.
But if we show things with an EmptyLocation, that will include things from outside user code and we'll get inconsistent results. It's also of dubious value if we don't know where to look for the problem.
There was a problem hiding this comment.
The MissingElement query is a bit special. It's purpose is to measure the quality of the database/extractor. If we throw out all MissingElements without location then we might think a database is in perfect health while in reality it is not.
There was a problem hiding this comment.
... and we're committed to not extracting non-user code now as I understand it (still now 100% sure how that works). I'll try including them again.
There was a problem hiding this comment.
I've just reverted the restriction to user code in this PR as well. I'm fairly sure I was seeing inconsistencies with it though, we will see...
There was a problem hiding this comment.
That reduces me to "Not yet implemented (x17)" locally.
There was a problem hiding this comment.
Did you comment out theprintln! in my_macro.rs as well as main.rs?
There was a problem hiding this comment.
... oops ...
Yep, that gets me down to 14, which is presumably in line with CI now. I've pushed the fix. There is still an issue in the extractor presumably, but this PR should be mergeable now I think.
There was a problem hiding this comment.
Thanks, yes there must be some issue, or perhaps the standard library for rust on the CI runners has a different implementation for println!.
There was a problem hiding this comment.
hmm, that is unfortunate. But maybe we just need to do a https://github.com/actions-rust-lang/setup-rust-toolchain step for running the QL tests?
This reverts commit aed44ba.
Done (provisionally assigned to you).
Will do... |
Done. |
Add Missing Elements diagnostic query (
rust/diagnostics/missing-elements) that listsUnimplemented,MissingExprandMissingPatelements. At the moment there's rather a lot, even on the simple test - probably mostly coming from the standard libraries. Future extractor changes should greatly reduce this.