no-shadowed-variable: Rewrite and lint for other shadowed names #2598

ajafff · 2017-04-18T18:13:58Z

PR checklist

Addresses an existing issue: Exempting "this" from the "no-shadowed-variable" rule #2214, Import declaration conflict not detected with import { Foo } from statement #2921
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

[enhancement] no-shadowed-variable: added checks for other shadowing declarations, e.g. interfaces, classes, type parameters, imports, etc.
Fixes: #2921
[bugfix] no-shadowed-variable: exempt this parameter
Fixes: #2214
[rule-change] no-shadowed-variable no longer fails for declarations in the same scope, e.g. var foo; var foo;. Use the rule no-duplicate-variable to find such errors.
Refs: #2597

Is there anything you'd like reviewers to focus on?

The compiler already handles declarations in the same scope pretty well. I don't think we should duplicate its logic if declarations are mergeable (or its error messages if they are not). If the user doesn't want declarations to merge, there's still no-duplicate-variable

This is my attempt to simplify #2339.
Closes: #2339
/cc @andy-hanson

andy-hanson · 2017-04-18T23:07:32Z

test/rules/no-shadowed-variable/test.ts.lint

+interface I {}
+interface I {}
+
+// Ignore non-mergeable declarations as the compiler will complain about them anyway


I don't see a compiler error (excepting the unused variable and implicit any) when I use that code. It's perfectly valid to write (function f1() { let f1: any; return f1; })();; the function f1 is shadowed.

good catch. I added some special handling for named function expressions

andy-hanson · 2017-04-18T23:11:17Z

src/rules/noShadowedVariableRule.ts

-    private addFailureOnIdentifier(ident: ts.Identifier) {
-        const failureString = Rule.FAILURE_STRING_FACTORY(ident.text);
-        this.addFailureAtNode(ident, failureString);
+function addToList(map: Map<string, ts.Identifier[]>, name: string, identifiers: ts.Identifier[]) {


Would make this more generic on T instead of ts.Identifier

adding a type parameter doesn't add anything as this function is always called with the same types

This function might need to be used elsewhere (multimaps show up often), but with a different type of values.

adidahiya · 2017-05-31T18:28:24Z

test/rules/no-shadowed-variable/test.ts.lint

+             ~ [err % ('C')]
+        namespace C {
+                  ~ [err % ('C')]
+            interface C<C> {}


now that we're linting for more than just shadowed variables, I'm tempted to change the rule name in the next major release to no-shadowed-name...

for now, can we at least distinguish the failure messages? users might get confused if they see "shadowed variable" for an interface name or a type parameter. let's use Shadowed variable: x for variables and Shadowed name: y for everything else

@adidahiya I changed the error message to always show Shadowed name: x.

Showing a different error message for variables is not really possible:

We're not tracking if an identifier is a variable

Variables could merge with classes, interfaces, namespaces ... are they still considered a variable

And even if we could distinguish the kind of identifiers, it's not clear when to use which message: if a variable shadows a type? if a type shadows a variable? only if a variable shadows another variable?

yeah good points 👍

ajafff · 2017-06-13T12:32:22Z

Added handling of imports to address #2921

adidahiya

overall lgtm

adidahiya · 2017-06-16T21:38:37Z

test/rules/no-shadowed-variable/test.ts.lint

@@ -198,8 +211,10 @@ function constructorsWithArgsParamConst(
    derivedCtor: new (...args: any[]) => any,
    baseCtors: Array<new (...args: any[]) => any>,
    args: any[]): void {
-    const args = [];
-          ~~~~       [Shadowed variable: 'args']
+    {


why the extra block here?

without the block there would be a compile error because the block scoped variable args cannot merge with the function scoped parameter args.
The block is needed here because the rule only handles shadowed names only in different scopes

adidahiya · 2017-06-16T21:45:15Z

test/rules/no-shadowed-variable/test.ts.lint

+             ~ [err % ('C')]
+        namespace C {
+                  ~ [err % ('C')]
+            interface C<C> {}


yeah good points 👍

Martinspire · 2017-07-10T08:48:02Z

So why is this suddenly marking all my classnames as Shadowed Name?

Shadowed name: 'SomeClassName' (no-shadowed-variable)
  37 |      * Some comment.
  38 |      */
> 39 |     export class SomeClassName implements SomeClassToImplement {
     |                 ^
  40 |         /**
  41 |          * Some more comments:


C:/<path>/<file>.ts
Shadowed name: 'SomeOtherClassName' (no-shadowed-variable)
  43 |      * Some comment.
  44 |      */
> 45 |     export class SomeOtherClassName implements SomeClassToImplement {
     |                 ^
  46 |         /**
  47 |          * Some more comments:


C:/<path>/<file>.ts
Shadowed name: 'AnotherClassName' (no-shadowed-variable)
  39 |      * Some comment.
  40 |      */
> 41 |     export class AnotherClassName implements SomeClassToImplement {
     |                 ^
  42 |         /**
  43 |          * Some more comments:

And if you look at how the classes are setup, it is like this:

import Something from "./something";

namespace SomeNamespace {

    /**
     * Comment about class
     */
    export class MyClass {
             <some functions, constructor and variables...>
        }
    }
}

import MyClass = SomeNamespace.MyClass;
export default MyClass;

I don't see how that would be faulty or shadowed
Right now it gives me 431 errors for absolutely no reason.

ajafff · 2017-07-10T12:59:30Z

@Martinspire import MyClass = SomeNamespace.MyClass; is shadowed by the declaration of MyClass inside the namespace. In your case both point to the same class. The rule does not handle this specific case.

For now you can just export default SomeNamespace.MyClass;.
There's an open issue #3000 that tracks adding options to ignore certain declarations

Martinspire · 2017-07-10T13:59:28Z

@ajafff but that doesn't work if you have multiple classes and interfaces to export. There can only be one default

Also I just found out that we're using it for Typedoc to separate by namespace so its easier to browse (though I also found a plugin to perhaps do that but, but I don't feel like changing so many files because this rule doesn't allow exclusions)

Martinspire · 2017-07-17T14:27:40Z

So for example if we want to keep using namespaces for typedoc, we use something like this:


export import ISomeClass = MyNameSpace.ISomeClass1;
export import ISomeClass2 = MyNameSpace.ISomeClass2;
export import ISomeClass3 = MyNameSpace.ISomeClass3;
export import ISomeClass4 = MyNameSpace.ISomeClass4;
import SomeDefaultClass = MyNameSpace.SomeDefaultClass;
export default SomeDefaultClass;

Inside other files I mostly only need SomeDefaultClass, but sometimes I also need the others and its impossible to get the others right now (unless you know another way)
And currently it is complaining, but there is no way to exclude this except for disabling the rule altogether

…ntir#2598)

andy-hanson reviewed Apr 18, 2017

View reviewed changes

ajafff added 4 commits May 12, 2017 22:53

no-shadowed-variable: Rewrite and lint for other shadowed names

dbef02f

Fix lint

b07c346

Fix lint

9f8b018

Merge branch 'master' into no-shadowed-variable

f2c063d

adidahiya mentioned this pull request May 24, 2017

no-duplicate-variable: handle parameters #2597

Merged

4 tasks

Merge branch 'master' into no-shadowed-variable

8340917

adidahiya suggested changes May 31, 2017

View reviewed changes

adidahiya added the PR: waiting for author label May 31, 2017

adidahiya mentioned this pull request May 31, 2017

Rewrite 'no-shadowed-variable' #2339

Closed

4 tasks

ajafff added 5 commits June 6, 2017 22:02

Rephrase error message

0fd3e95

Merge branch 'master' into no-shadowed-variable

976397d

Merge branch 'master' into no-shadowed-variable

92d3a32

Also check imports

2aaf60d

Fix lint

a533e36

ajafff mentioned this pull request Jun 13, 2017

Import declaration conflict not detected with import { Foo } from statement #2921

Closed

adidahiya added PR: waiting for reviewer and removed PR: waiting for author labels Jun 13, 2017

adidahiya approved these changes Jun 16, 2017

View reviewed changes

adidahiya removed the PR: waiting for reviewer label Jun 16, 2017

adidahiya merged commit 08ccd47 into palantir:master Jun 16, 2017

renovate bot mentioned this pull request Jul 5, 2017

fix: Update dependency tslint to version 5.5.0- singapore/lint-condo#327

Closed

rokoroku mentioned this pull request Jul 6, 2017

no-shadowed-variable: please add scope options #3000

Closed

This was referenced Jul 6, 2017

Update dependency tslint to version 5.5.0- singapore/lint-condo#328

Closed

chore(deps): Update dependency tslint to version 5.5.0 krjordan/brewski-catalogue#9

Closed

ajafff deleted the no-shadowed-variable branch July 10, 2017 10:30

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

no-shadowed-variable: Rewrite and lint for other shadowed names (pala…

8db9c90

…ntir#2598)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

no-shadowed-variable: Rewrite and lint for other shadowed names #2598

no-shadowed-variable: Rewrite and lint for other shadowed names #2598

ajafff commented Apr 18, 2017 •

edited

andy-hanson Apr 18, 2017 •

edited

ajafff Apr 19, 2017

andy-hanson Apr 18, 2017

ajafff Apr 19, 2017

andy-hanson Apr 19, 2017

adidahiya May 31, 2017

ajafff Jun 6, 2017

adidahiya Jun 16, 2017

ajafff commented Jun 13, 2017

adidahiya left a comment

adidahiya Jun 16, 2017

ajafff Jun 18, 2017

adidahiya Jun 16, 2017

Martinspire commented Jul 10, 2017 •

edited

ajafff commented Jul 10, 2017

Martinspire commented Jul 10, 2017 •

edited

Martinspire commented Jul 17, 2017 •

edited

no-shadowed-variable: Rewrite and lint for other shadowed names #2598

no-shadowed-variable: Rewrite and lint for other shadowed names #2598

Conversation

ajafff commented Apr 18, 2017 • edited

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

andy-hanson Apr 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Jun 13, 2017

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinspire commented Jul 10, 2017 • edited

ajafff commented Jul 10, 2017

Martinspire commented Jul 10, 2017 • edited

Martinspire commented Jul 17, 2017 • edited

ajafff commented Apr 18, 2017 •

edited

andy-hanson Apr 18, 2017 •

edited

Martinspire commented Jul 10, 2017 •

edited

Martinspire commented Jul 10, 2017 •

edited

Martinspire commented Jul 17, 2017 •

edited