fix: stop JavaScript renaming a private member its references still name [patch] - #70
Merged
Merged
Conversation
…ame [patch] `JavaScriptGenerator.MemberName` prefixed `#` onto a member declared `Visibility.Private`, and nothing prefixed the references to it — a `VariableReference` is emitted verbatim by the shared path every generator uses. A class with a private field and a method that reads it came out declaring `#count` and reading `count`, which resolves to nothing and throws a `ReferenceError` under a module's implicit strict mode. The file was invalid and the failure was silent at generation time. Visibility is now dropped rather than spelled. That is the same answer `PythonGenerator` gives for its leading underscore and `GoGenerator` gives for the case of a name, for the same stated reason, and the same one this generator already gave for a property: renaming a declaration here leaves every reference to it naming something that no longer exists. `VisibilityTests` pinned the declaration side only, which is why this passed. It now generates a private member *and* a reference to it, so the two halves cannot drift apart again. The two tests asserting `static #name` keep their subject — that `static` survives — and assert the name is not rewritten alongside it. Fixes #34 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hhysM4RPfAapVsCatxD5f
…anged lines SonarCloud's MSTEST0046 flagged the five assertions this branch touched. `FunctionModifierTests` already uses the preferred form, so this is the convention the suite is moving to rather than a new one. The negative assertions become `Assert.DoesNotContain` alongside them, which reports the offending output on failure instead of just "expected false". Pre-existing `StringAssert` calls on lines this branch does not touch are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hhysM4RPfAapVsCatxD5f
|
This file contains hidden or 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
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.



Fixes #34
What was wrong
JavaScriptGenerator.MemberNameprefixed#onto a member declaredVisibility.Private, and nothing prefixed the references to it — aVariableReferenceis emitted verbatim by the shared path every generator uses. The declaration was renamed and every reference to it was not:Under a module's implicit strict mode that is a
ReferenceErrorat run time. The generated file was not usable, and the failure was silent at generation time.The fix
Visibility is dropped rather than spelled, which is option 1 in the issue and what triage recommended.
Three places in this codebase had already reached that conclusion from the same observation, which is what makes it the consistent answer rather than the merely smaller one:
PythonGeneratordeclines its leading-underscore convention because "renaming a declaration here would leave everyVariableReferenceto it naming something that no longer exists"GoGeneratorwrites a note about the case of a name rather than changing it, "renaming would leave every reference to the old name behind"CGeneratorleaves a namespace as a comment, since "folding its name into the declarations would rename them without renaming the references to them"JavaScriptGeneratoritself already said so too — itsGeneratePropertyDeclarationremark reads "Visibility is not written. A JavaScript member is private only if its name begins with a#, which is a rename rather than a modifier, and this generator renames nothing" — which was true of the property path and not of the field and method paths beside it. The generator now says one thing throughout.The cost is that JavaScript output does not express privacy, which is the same cost Python and Go already pay here.
Tests
VisibilityTestspinned the declaration side only —#x = 0;and#recompute() {— which is why this passed. AddedJavaScript_ReferencesThePrivateMemberItDeclared, which generates a private member and a body that reads it and asserts the two name the same identifier, so the halves cannot drift apart again.Verified it catches the bug rather than merely passing: restoring the pre-fix
JavaScriptGenerator.csfrom the base commit and re-running it fails onAssert.DoesNotContain("#count", code), reporting the defect exactly —Three existing tests asserted the old spelling and were updated. Two of them —
JavaScript_CombinesStaticWithAPrivateNameandJavaScript_WritesStaticAlongsideThePrivatePrefix— were really aboutstaticsurviving next to the name, so they keep that subject and now also assert the name is not rewritten alongside it.Commits
fix:the generator change, the new regression test, the three updated tests, and the README rows.style(test):SonarCloud's MSTEST0046 flagged the five assertions this branch touched —Assert.Containsis preferred overStringAssert.Contains, andFunctionModifierTestsalready uses that form. Converted those five, with the negative assertions becomingAssert.DoesNotContainso a failure reports the offending output instead of just "expected false". Pre-existingStringAssertcalls on lines this branch does not touch are left alone.Verification
dotnet build Coder.sln -c Release: 0 warnings, 0 errorsDocs
Two README rows described the
#behaviour and now describe the current one.CHANGELOG.md/VERSION.md/AUTHORS.mdare pipeline-generated and untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_013hhysM4RPfAapVsCatxD5f