Fix incorrect flags in modifierFlagsCache

Author: rbuckton

src/compiler/types.ts

@@ -863,13 +863,14 @@ export const enum ModifierFlags {
     Async =              1 << 9,  // Property/Method/Function
     Default =            1 << 10, // Function/Class (export default declaration)
     Const =              1 << 11, // Const enum
-    HasComputedJSDocModifiers = 1 << 12, // Indicates the computed modifier flags include modifiers from JSDoc.
-
-    Deprecated =         1 << 13, // Deprecated tag.
-    Override =           1 << 14, // Override method.
-    In =                 1 << 15, // Contravariance modifier
-    Out =                1 << 16, // Covariance modifier
-    Decorator =          1 << 17, // Contains a decorator.
+    Deprecated =         1 << 12, // Deprecated tag.
+    Override =           1 << 13, // Override method.
+    In =                 1 << 14, // Contravariance modifier
+    Out =                1 << 15, // Covariance modifier
+    Decorator =          1 << 16, // Contains a decorator.
+
+    HasComputedJSDocModifiers = 1 << 27, // Indicates the computed modifier flags include modifiers from JSDoc.
+    HasExcessJDocModifiers = 1 << 28, // Indicates that there are JSDoc modifiers that are not in the cache and must be recalculated. For use with nodes in TS files only.
     HasComputedFlags =   1 << 29, // Modifier flags have been computed
 
     AccessibilityModifier = Public | Private | Protected,
@@ -878,6 +879,7 @@ export const enum ModifierFlags {
     NonPublicAccessibilityModifier = Private | Protected,
 
     TypeScriptModifier = Ambient | Public | Private | Protected | Readonly | Abstract | Const | Override | In | Out,
+    JSDocOnlyModifier = Deprecated,
     ExportDefault = Export | Default,
     All = Export | Ambient | Public | Private | Protected | Static | Readonly | Abstract | Accessor | Async | Default | Const | Deprecated | Override | In | Out | Decorator,
     Modifier = All & ~Decorator,

src/compiler/utilities.ts

@@ -6924,15 +6924,45 @@ function getModifierFlagsWorker(node: Node, includeJSDoc: boolean, alwaysInclude
         return ModifierFlags.None;
     }
 
+    // Calculate the modifier flags for a given node, and cache them based on the file containing the node.
+    // For a TypeScript file, the cache will only contain syntactic modifiers and potentially the `Deprecated` JSDoc modifier.
+    // For a JavaScript file, the cache will contain both syntactic and JSDoc modifiers.
+
     if (!(node.modifierFlagsCache & ModifierFlags.HasComputedFlags)) {
         node.modifierFlagsCache = getSyntacticModifierFlagsNoCache(node) | ModifierFlags.HasComputedFlags;
     }
 
-    if (includeJSDoc && !(node.modifierFlagsCache & ModifierFlags.HasComputedJSDocModifiers) && (alwaysIncludeJSDoc || isInJSFile(node)) && node.parent) {
-        node.modifierFlagsCache |= getJSDocModifierFlagsNoCache(node) | ModifierFlags.HasComputedJSDocModifiers;
+    let jsdocModifierFlags: ModifierFlags | undefined;
+    if (includeJSDoc && !(node.modifierFlagsCache & ModifierFlags.HasComputedJSDocModifiers) && node.parent) {
+        jsdocModifierFlags = getJSDocModifierFlagsNoCache(node);
+        if (isInJSFile(node)) {
+            node.modifierFlagsCache |= jsdocModifierFlags | ModifierFlags.HasComputedJSDocModifiers;
+        }
+        else {
+            node.modifierFlagsCache |= (jsdocModifierFlags & ModifierFlags.Deprecated) | ModifierFlags.HasComputedJSDocModifiers;
+            if (jsdocModifierFlags & ~node.modifierFlagsCache & ~ModifierFlags.Deprecated) {
+                // If there are other JSDoc modifiers aside from `@deprecated` that aren't also in the set of syntactic modifiers for the node,
+                // we can add a flag indicating we need to recompute JSDoc modifiers when `alwaysIncludeJSDoc` is true.
+                node.modifierFlagsCache |= ModifierFlags.HasExcessJDocModifiers;
+            }
+        }
+    }
+    
+    const modifiers = node.modifierFlagsCache & ~(ModifierFlags.HasComputedFlags | ModifierFlags.HasComputedJSDocModifiers | ModifierFlags.HasExcessJDocModifiers);
+    if (includeJSDoc || alwaysIncludeJSDoc) {
+        if (!isInJSFile(node) && alwaysIncludeJSDoc && node.modifierFlagsCache & ModifierFlags.HasExcessJDocModifiers) {
+            // If we are requesting JSDoc modifiers in a TS file, we only need to recalculate additional JSDoc modifiers if the cache indicates there are any to find.
+            // We recalculate the JSDoc modifiers since they may overlap with the syntactic modifiers in the cache. While this isn't as efficient as caching the modifiers,
+            // it is rare to use JSDoc for modifiers like `@private` in a TS file.
+            jsdocModifierFlags ??= getJSDocModifierFlagsNoCache(node);
+            return modifiers | jsdocModifierFlags;
+        }
+        return modifiers;
+    }
+    else {
+        // We are requesting only syntactic modifiers. filter out any JSDoc modifiers in the cache.
+        return (isInJSFile(node) ? modifiers & ~ModifierFlags.TypeScriptModifier : modifiers) & ~ModifierFlags.JSDocOnlyModifier;
     }
-
-    return node.modifierFlagsCache & ~(ModifierFlags.HasComputedFlags | ModifierFlags.HasComputedJSDocModifiers);
 }
 
 /**

src/testRunner/unittests/publicApi.ts

@@ -231,3 +231,34 @@ describe("unittests:: Public APIs:: getChild* methods on EndOfFileToken with JSD
         assert.notEqual(endOfFileToken.getChildAt(0), /*expected*/ undefined);
     });
 });
+
+describe("unittests:: Public APIs:: get syntactic and effective modifiers", () => {
+    it("caches and reports correct flags in TS file", () => {
+        // https://github.com/microsoft/TypeScript/issues/42189
+        const content = `
+class C {
+    /** @private */
+    prop = 1;
+}`;
+        const sourceFile = ts.createSourceFile("/file.ts", content, ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+        const classNode = sourceFile.statements[0] as ts.ClassDeclaration;
+        const propNode = classNode.members[0] as ts.PropertyDeclaration;
+        assert.equal(ts.ModifierFlags.None, ts.getSyntacticModifierFlags(propNode));
+        assert.equal(ts.ModifierFlags.None, ts.getEffectiveModifierFlags(propNode));
+        assert.equal(ts.ModifierFlags.None, ts.getSyntacticModifierFlags(propNode));
+    });
+    it("caches and reports correct flags in JS file", () => {
+        // https://github.com/microsoft/TypeScript/issues/42189
+        const content = `
+class C {
+    /** @private */
+    prop = 1;
+}`;
+        const sourceFile = ts.createSourceFile("/file.js", content, ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+        const classNode = sourceFile.statements[0] as ts.ClassDeclaration;
+        const propNode = classNode.members[0] as ts.PropertyDeclaration;
+        assert.equal(ts.ModifierFlags.None, ts.getSyntacticModifierFlags(propNode));
+        assert.equal(ts.ModifierFlags.Private, ts.getEffectiveModifierFlags(propNode));
+        assert.equal(ts.ModifierFlags.None, ts.getSyntacticModifierFlags(propNode));
+    });
+});

tests/baselines/reference/api/typescript.d.ts

@@ -4764,20 +4764,22 @@ declare namespace ts {
         Async = 512,
         Default = 1024,
         Const = 2048,
-        HasComputedJSDocModifiers = 4096,
-        Deprecated = 8192,
-        Override = 16384,
-        In = 32768,
-        Out = 65536,
-        Decorator = 131072,
+        Deprecated = 4096,
+        Override = 8192,
+        In = 16384,
+        Out = 32768,
+        Decorator = 65536,
+        HasComputedJSDocModifiers = 134217728,
+        HasExcessJDocModifiers = 268435456,
         HasComputedFlags = 536870912,
         AccessibilityModifier = 28,
-        ParameterPropertyModifier = 16476,
+        ParameterPropertyModifier = 8284,
         NonPublicAccessibilityModifier = 24,
-        TypeScriptModifier = 117086,
+        TypeScriptModifier = 59742,
+        JSDocOnlyModifier = 4096,
         ExportDefault = 1025,
-        All = 258047,
-        Modifier = 126975,
+        All = 131071,
+        Modifier = 65535,
     }
     enum JsxFlags {
         None = 0,

tests/baselines/reference/jsFileCompilationPublicParameterModifier.symbols

@@ -3,5 +3,5 @@
 === a.js ===
 class C { constructor(public x) { }}
 >C : Symbol(C, Decl(a.js, 0, 0))
->x : Symbol(C.x, Decl(a.js, 0, 22))
+>x : Symbol(x, Decl(a.js, 0, 22))