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

getSyntacticModifierFlags return value affected by getEffectiveModifierFlags #42189

Closed
ajafff opened this issue Jan 3, 2021 · 1 comment · Fixed by #56198
Closed

getSyntacticModifierFlags return value affected by getEffectiveModifierFlags #42189

ajafff opened this issue Jan 3, 2021 · 1 comment · Fixed by #56198
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@ajafff
Copy link
Contributor

ajafff commented Jan 3, 2021

Bug Report

I don't know if this already causes a user-facing bug, but it might in the future.

🔎 Search Terms

syntactic effective modifierflags

🕗 Version & Regression Information

The API was introduced in #38403 by @rbuckton on 12th May 2020, I don't know which TS version that is

💻 Code

class C {
  /** @private */
  prop = 1;
}

The code snippets below assume that node: ts.PropertyDeclaration of property prop

🙁 Actual behavior

Note: I'm not talking about user code using compiler API as these APIs are not public. I'm talking about the compiler implementation itself.

If the above code is a TS file:

  • call ts.getSyntacticModifierFlags(node), return ts.ModifierFlags.None (expected)
  • call ts.getEffectiveModifierFlags(node), return ts.ModifierFlags.None (expected)
  • call ts.getSyntacticModifierFlags(node) again, returns ts.ModifierFlags.None (expected)

If the above code is a JS file:

  • call ts.getSyntacticModifierFlags(node), return ts.ModifierFlags.None (expected)
  • call ts.getEffectiveModifierFlags(node), return ts.ModifierFlags.Private (expected)
  • call ts.getSyntacticModifierFlags(node) again, returns ts.ModifierFlags.Private (not expected)

(see https://runkit.com/ajafff/5ff204e2ac3559001a1407f7)

This is because both use the same property to cache the result. getEffectiveModifierFlags adds modifiers from JSDoc to the cached result. Calling getSyntacticModifierFlags afterwards cannot remove the modifiers from JSDoc, so it simply returns them too.

🙂 Expected behavior

Looking at the description of #38403, the intended use of getSyntacticModifierFlags is: "get me the syntactic modifiers and don't include those from JSDoc". Calling getEffectiveModifierFlags should not have a side-effect on getSyntacticModifierFlags.

Also there's getEffectiveModifierFlagsAlwaysIncludeJSDoc introduced in #38523 by @Kingwl. This affects TS files, making the original change in #38403 useless depending on execution order:

  • call ts.getSyntacticModifierFlags(node), return ts.ModifierFlags.None (expected)
  • call ts.getEffectiveModifierFlags(node), return ts.ModifierFlags.None (expected)
  • call getEffectiveModifierFlagsAlwaysIncludeJSDoc, return ts.ModifierFlags.Private (expected)
  • call ts.getSyntacticModifierFlags(node) again, returns ts.ModifierFlags.Private (not expected)
  • call ts.getEffectiveModifierFlags(node) again, returns ts.ModifierFlags.Private (not expected)
@Kingwl
Copy link
Contributor

Kingwl commented Jan 3, 2021

Good catch! definitely a bug.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jan 4, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.3.0 milestone Jan 4, 2021
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Jun 18, 2021
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants