JS: Add library for naming endpoints #15380
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
asgerf
force-pushed
the
js/endpoint-naming
branch
2 times, most recently
from
8842c18 to
930d79d
Compare
asgerf
force-pushed
the
js/endpoint-naming
branch
3 times, most recently
from
17263c5 to
08e61b6
Compare
asgerf
force-pushed
the
js/endpoint-naming
branch
from
08e61b6 to
36e069b
Compare
asgerf
force-pushed
the
js/endpoint-naming
branch
from
36e069b to
1737ba1
Compare
erik-krogh
reviewed
Jan 31, 2024
There was a problem hiding this comment.
Since a given endpoint can very often be reached via multiple access paths (some of which are very ugly) a great deal of work goes into ranking these access paths and try to pick the least bad best one.
Looks like you did a lot of iterations of making it slightly better.
Nice work 👍
I got some comments, but they're all minor.
javascript/ql/test/testUtilities/internal/InlineExpectationsTestImpl.qll
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/endpoints/EndpointNaming.qll
Outdated
Show resolved
Hide resolved
Previously it was theoretically possible to create a cycle of preferred predecessors, since badness had higher precedence than depth. We now require the preferred predecessor to have lower depth. With this criteria we can remove the arbitray cap on badness.
|
Thanks for the review @erik-krogh, I've answered all comments, PTAL. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a library,
EndpointNaming.qllfor generating human-readable names for endpoints, based on their access paths.Since a given endpoint can very often be reached via multiple access paths (some of which are very ugly) a great deal of work goes into ranking these access paths and try to pick the
least badbest one.There are several intended use-cases for this. The model editor and VEA both need to generate human-readable names for endpoints. Also, for library summarisation we need to be able to name intermediate types and good readability for those names not a strict requirement though certainly a nice-to-have.
There are some unit tests in the PR, and some more real-world test output stored in a separate repo (internal link).
The work isn't entirely done yet, though I think it would be helpful to get it reviewed and merged in the current state, and then iterate from there.
Commit-by-commit review recommended as there are some general-purpose initial commits.