Fix false-positive TS2354 for native private class field access with importHelpers at dated targets - #63729
Closed
Andrew Stegmaier (astegmaier) wants to merge 1 commit into
Conversation
…ted targets checkPropertyAccessExpressionOrQualifiedName, checkInExpression, and setNodeLinksForPrivateIdentifierScope gated the `tslib` helper requirement for private-identifier access on `languageVersion < LanguageFeatureMinimumTarget.ClassAndClassElementDecorators` in addition to the private-fields-specific threshold. Since decorators have never been assigned a dated ECMAScript edition, ClassAndClassElementDecorators is pinned to the ESNext sentinel, so this condition was true for every dated target (ES2022 through ES2025) unconditionally - regardless of whether the file used decorators at all. This caused `tsc` to require `tslib` (and hard error with TS2354 if it couldn't be resolved) for plain, fully-native private field/method/accessor access, even though the actual emitted JS never references tslib for these constructs (the emitter's shouldTransformPrivateElementsOrClassStaticBlocks in classFields.ts only checks languageVersion < ES2022). The `!useDefineForClassFields` clause on the same three checks had the same problem: private fields are always emitted using 'define' semantics regardless of useDefineForClassFields (which only affects public fields), so it doesn't correspond to any real difference in whether tslib is needed for private-field access. The fourth call site with the same ClassAndClassElementDecorators check, inside getFirstTransformableStaticClassElement, is left unchanged: it's used only to decide whether a *decorated* class needs the __setFunctionName helper for hoisting its static private/static-block elements, which is a genuine, verified coupling between decorators and private statics. Fixes microsoft#63728 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
I'm closing this PR per Ryan Cavanaugh (@RyanCavanaugh)'s advice that the only thing that's appropriate to go into the 6.0 branch is security fixes - and this is definitely not that. |
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 #63728.
The bug
With
importHelpers: trueand a datedtarget(e.g.ES2022–ES2025),tscincorrectly reportsTS2354: This syntax requires an imported helper but module 'tslib' cannot be foundfor plain, fully-native private field/method/accessor access (this.#x,#x in obj, static private fields/methods, private accessors, static blocks, private fields in generics/closures/class expressions/inheritance) — even though the emitted JS for all of these is 100% native ES2022+ syntax that never referencestslib.Root cause
checkPropertyAccessExpressionOrQualifiedName,checkInExpression, andsetNodeLinksForPrivateIdentifierScopegate the private-identifier helper-requirement check on:ClassAndClassElementDecoratorsis pinned to theESNextsentinel because TC39 decorators have never been assigned to a dated ECMAScript edition. That makeslanguageVersion < ClassAndClassElementDecoratorstruefor every dated target, forever — regardless of whether the file uses decorators at all. The actual emitter (classFields.ts'sshouldTransformPrivateElementsOrClassStaticBlocks) only checkslanguageVersion < ES2022, with no decorator-related gating, so this term in the checker never corresponded to real emit behavior.!useDefineForClassFieldshas the same problem: private fields are always emitted using "define" semantics regardless ofuseDefineForClassFields(that flag only affects public fields), so it also doesn't correspond to any real difference in whethertslibis needed for private-field access. Verified by emitting with--useDefineForClassFields false --target es2022: output is fully native with notslibreferences, yet the checker still errored before this fix.The fix
Removes both spurious clauses from the 3 call sites above, leaving just
languageVersion < LanguageFeatureMinimumTarget.PrivateNamesAndClassStaticBlocks.A 4th call site with the same
ClassAndClassElementDecoratorscheck, insidegetFirstTransformableStaticClassElement, is intentionally left unchanged. That one only matters when a class is decorated, to decide if the decorator transform's hoisting of static private/static-block elements into an IIFE needs the__setFunctionNamehelper. I verified (by emitting@dec class C { static #foo() {} }at ES2022 with--outDir) that this really is required in the actual output, so removing this clause caused a real regression inesDecorators-classDeclaration-missingEmitHelpers-classDecorator.3.ts; that edit was reverted.Testing
tests/cases/compiler/importHelpersNoHelpersForPrivateFieldsAtES2022.ts— target ES2022,importHelpers: true, notslibpresent, covering instance/static private fields, private methods, static private methods, private accessors, private auto-accessors (instance + static), static blocks, and#x in obj. Asserts zero errors; baseline.jsconfirms the emit never referencestslib.tests/cases/compiler/importHelpersNoHelpersForPrivateFieldsAtES2022UseDefineForClassFieldsFalse.ts— same coverage withuseDefineForClassFields: false, confirming that flag no longer triggers the false positive either.hereby runtests-parallel --light=false, 106,383 tests) — all passing, no baseline regressions.hereby lintpasses.usingdeclarations still correctly requiretslibat dated targets (these are real transforms, not part of this bug).using) are unaffected.