Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Expose whether an endpoint name is synthetic #15975

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 19, 2024

The EndpointNaming library was written to support two use-cases: the model editor, and VEA.

However, I think they differ in how they should deal with the "semi-internal class" problem:

class InternalClass {
  m() {} // exposed to client code, but not reachable via an access path
}
export function get() {
  return new InternalClass();
}

For the model editor use-case, it seemed best to synthesise a name for InternalClass.m() above, rather than give up entirely.

This is not ideal for the other use-case however, so this change exposes a boolean parameter to know whether the name is synthetic or not.

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Mar 19, 2024
@github-actions github-actions bot added the JS label Mar 19, 2024
@asgerf asgerf marked this pull request as ready for review March 19, 2024 14:46
@asgerf asgerf requested a review from a team as a code owner March 19, 2024 14:46
@@ -236,7 +240,7 @@
*
* `badness` is bound to the associated badness of the name.
*/
private predicate sinkHasPrimaryName(API::Node sink, string package, string name, int badness) {
private predicate sinkHasPrimaryNameAux(API::Node sink, string package, string name, int badness) {

Check warning

Code scanning / CodeQL

Candidate predicate not marked as `nomagic` Warning

Candidate predicate to
sinkHasPrimaryName
is not marked as nomagic.
Candidate predicate to
sinkHasPrimaryName
is not marked as nomagic.
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks OK to me. (but all this code is new to me)

I wonder if fallbackModuleName should also be considered a synthetic name? Currently it's only used for classHasFallbackName, so it doesn't have any immediate impact, but I'm just wondering if we could end up in a situation where using fallbackModuleName only gives 50 in penalty, so we actually synthetize a name without treating it as synthezied?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants