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
fix: fix static private field shadowed by local variable #13656
Changes from 1 commit
80b4d84
d01e651
8de2f46
5c366b3
68decd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,7 @@ export function buildPrivateNamesMap(props: PropPath[]) { | |
export function buildPrivateNamesNodes( | ||
privateNamesMap: PrivateNamesMap, | ||
privateFieldsAsProperties: boolean, | ||
state, | ||
state: File, | ||
) { | ||
const initNodes: t.Statement[] = []; | ||
|
||
|
@@ -165,6 +165,7 @@ interface PrivateNameState { | |
classRef: t.Identifier; | ||
file: File; | ||
noDocumentAll: boolean; | ||
innerBinding?: t.Identifier; | ||
} | ||
|
||
const privateNameVisitor = privateNameVisitorFactory< | ||
|
@@ -255,7 +256,7 @@ const privateNameHandlerSpec: Handler<PrivateNameState & Receiver> & Receiver = | |
}, | ||
|
||
get(member) { | ||
const { classRef, privateNamesMap, file } = this; | ||
const { classRef, privateNamesMap, file, innerBinding } = this; | ||
const { name } = (member.node.property as t.PrivateName).id; | ||
const { | ||
id, | ||
|
@@ -273,6 +274,13 @@ const privateNameHandlerSpec: Handler<PrivateNameState & Receiver> & Receiver = | |
? "classStaticPrivateMethodGet" | ||
: "classStaticPrivateFieldSpecGet"; | ||
|
||
const binding = member.scope.getBinding(classRef.name); | ||
if (innerBinding && binding && !(binding.identifier === innerBinding)) { | ||
// the classRef has been shadowed | ||
throw binding.path.buildCodeFrameError( | ||
`Shadowing class ${classRef.name} with private property`, | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd rename the local variable: path.scope.rename(classRef.name); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! I renamed all the local variables that shadowing the |
||
} | ||
return t.callExpression(file.addHelper(helperName), [ | ||
this.receiver(member), | ||
t.cloneNode(classRef), | ||
|
@@ -463,8 +471,8 @@ export function transformPrivateNamesUsage( | |
ref: t.Identifier, | ||
path: NodePath<t.Class>, | ||
privateNamesMap: PrivateNamesMap, | ||
{ privateFieldsAsProperties, noDocumentAll }, | ||
state, | ||
{ privateFieldsAsProperties, noDocumentAll, innerBinding }, | ||
state: File, | ||
) { | ||
if (!privateNamesMap.size) return; | ||
|
||
|
@@ -479,6 +487,7 @@ export function transformPrivateNamesUsage( | |
file: state, | ||
...handler, | ||
noDocumentAll, | ||
innerBinding, | ||
}); | ||
body.traverse(privateInVisitor, { | ||
privateNamesMap, | ||
|
@@ -770,7 +779,7 @@ const thisContextVisitor = traverse.visitors.merge([ | |
]); | ||
|
||
const innerReferencesVisitor = { | ||
ReferencedIdentifier(path, state) { | ||
ReferencedIdentifier(path: NodePath<t.Identifier>, state) { | ||
if ( | ||
path.scope.bindingIdentifierEquals(path.node.name, state.innerBinding) | ||
) { | ||
|
@@ -831,7 +840,7 @@ export function buildFieldsInitNodes( | |
superRef: t.Expression | undefined, | ||
props: PropPath[], | ||
privateNamesMap: PrivateNamesMap, | ||
state, | ||
state: File, | ||
setPublicClassFields: boolean, | ||
privateFieldsAsProperties: boolean, | ||
constantSuper: boolean, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
class Test { | ||
|
||
static #x = 1 | ||
|
||
method() { | ||
const Test = 1; | ||
return this.#x; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"plugins": ["proposal-class-properties"], | ||
"throws": "Shadowing class Test with private property" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
Also,
binding
should always be defined (because if there isn't a conflicting variable, it'sinnerBinding
). It would be better to assert it, rather than checking it:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Infect,
binding
could be undefined.e.g: for a
ClassExpression
,classRef
is generated bypath.scope.generateUidIdentifier("class");
and would be inserted after our transformation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks, I didn't realize it