Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

new rule: Respect static member privacy #169

Closed
leemiro opened this issue Jul 5, 2016 · 6 comments
Closed

new rule: Respect static member privacy #169

leemiro opened this issue Jul 5, 2016 · 6 comments
Labels
Microsoft Internal Issues related to closed source Microsoft code. 💀 R.I.P. 💀 Requires Type Checker Must be implemented with a "typed" rule that uses a TypeScript program. Status: Accepting PRs Type: Rule Suggestion Adding a new rule that doesn't yet exist here or in TSLint core.

Comments

@leemiro
Copy link

leemiro commented Jul 5, 2016

TypeScript behaves unintuitively when a base class references its private static variable via a derived class, as follows:

class FooBase {
    private static privateStatic: string = "unset";

    testPrivateStatic(): void {
        Foo.privateStatic = "set";
        console.log(FooBase.privateStatic);
   }
}

class Foo extends FooBase { }

// Logs "unset" but you'd expect it to log "set"
new FooBase().testPrivateStatic();

There is an issue open against TypeScript here, but until that's fixed (and after, for older versions of TypeScript), we should have a rule that prohibits referencing derived classes' private statics since the behavior of doing such is likely not what the developer expects.

@HamletDRC
Copy link
Member

HamletDRC commented Jul 6, 2016

One thing to keep in mind when discussing this is that tslint runs the linter on one file at a time and not on the program as a whole. So in general, there is almost no type information available within a rule. See palantir/tslint#680

What we /can/ do to each class we lint is this:

  • create a list of all private static field names (in this case "privateStatic")
  • create violations for what look like writes to that field but through a different class reference:
    • Look for any PropertyAccess that ends in a known private field name ("privateStatic")
    • Test to make sure the target of the PropertyAccess is not the current class (not "FooBase")
    • Test to make sure the target of the PropertyAccess starts with a capital letter ("Foo" and not "foo")
    • Test to make sure that this is a write to that field (look for BinaryExpressions with the EqualsOperator)

This will result in false positives, of course. But there is no way to ask the linter if one class is a subclass of another. The false positive scenario would be:

class Foo {
    private static privateStatic: string = "unset";
    testPrivateStatic(): void {
        Bar.privateStatic = "set"; // Bar is not a subclass of Foo, but would trigger a violation! 
   }
}

class Bar {
    private static privateStatic: string = "whatever";
}

Given these limitations, do you still want the rule?

@leemiro
Copy link
Author

leemiro commented Jul 6, 2016

It could be that it's worth waiting for #680, since that'll make this a lot easier to implement accurately.

Even using the steps you proposed above would be valuable, since the false positive example you provided is illegal and should get caught by the TS compiler anyways (referencing other classes' private members is not allowed). It would concern me if this flagged valid code.

@HamletDRC
Copy link
Member

Sorry, I made a mistake in my example. Imagine that Bar has a public field named "privateStatic":

class Bar {
    public static privateState: string = "whatever";
}

@leemiro
Copy link
Author

leemiro commented Jul 7, 2016

I see. Okay, then this would have to wait until palantir/tslint#680 is complete. Thanks for the feedback!

@HamletDRC HamletDRC added enhancement Requires Type Checker Must be implemented with a "typed" rule that uses a TypeScript program. Microsoft Internal Issues related to closed source Microsoft code. and removed enhancement labels Jul 9, 2016
@JoshuaKGoldberg
Copy link

palantir/tslint#680 is complete! 🙌

@JoshuaKGoldberg
Copy link

💀 It's time! 💀

TSLint is being deprecated; therefore, it and tslint-microsoft-contrib are no longer accepting pull requests for major new changes or features. See palantir/tslint#4534. 😱

If you'd like to see this change implemented, you have two choices:

  • Recommended: Check if this is available in ESLint + typescript-eslint
  • Not Recommended: Fork TSLint or tslint-microsoft-contrib locally 🤷‍♂️

👋 It was a pleasure open sourcing with you!

If you believe this message was posted here in error, please comment so we can re-open the issue!

@JoshuaKGoldberg JoshuaKGoldberg added Type: Rule Suggestion Adding a new rule that doesn't yet exist here or in TSLint core. 💀 R.I.P. 💀 labels Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Microsoft Internal Issues related to closed source Microsoft code. 💀 R.I.P. 💀 Requires Type Checker Must be implemented with a "typed" rule that uses a TypeScript program. Status: Accepting PRs Type: Rule Suggestion Adding a new rule that doesn't yet exist here or in TSLint core.
Projects
None yet
Development

No branches or pull requests

3 participants