Update: fix no-restricted-imports importNames reporting (fixes #12282) #12711

fanich37 · 2019-12-25T15:20:30Z

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix (it should fix #12282)

What changes did you make? (Give an overview)
Now rule can produce multiple errors depends on importNames option. This change allows applying directive comments on each item imported from the module.

Is there anything you'd like reviewers to focus on?
Yes, the test cases since i'm too bad at it.

jsf-clabot · 2019-12-25T15:20:32Z

All committers have signed the CLA.

mdjermanovic

Thanks for the PR! I didn't check everything, just noticed one change and have a question about that.

lib/rules/no-restricted-imports.js

mdjermanovic

I left some notes about * and default.

Perhaps we should also consider refactoring the rule.

The actual flow in this PR is:

checkNode() collects importNames and calls isRestrictedPath() to find out is the import invalid.
isRestrictedPath() uses importNames and configuration to return true or false.
If it's true, reportPath() is called. Then, this function is supposed to report something, and do it with the precise locations.

The problem is - reportPath() doesn't get any useful info from the previous steps, apart from knowing that there's something invalid because it's being called? So, it basically repeats steps 1 and 2., meaning that these steps were almost useless. Maybe we should do everything in checkNode(), instead.

lib/rules/no-restricted-imports.js

fanich37 · 2020-01-02T14:25:56Z

I left some notes about * and default.

Perhaps we should also consider refactoring the rule.

The actual flow in this PR is:

checkNode() collects importNames and calls isRestrictedPath() to find out is the import invalid.

isRestrictedPath() uses importNames and configuration to return true or false.

If it's true, reportPath() is called. Then, this function is supposed to report something, and do it with the precise locations.

The problem is - reportPath() doesn't get any useful info from the previous steps, apart from knowing that there's something invalid because it's being called? So, it basically repeats steps 1 and 2., meaning that these steps were almost useless. Maybe we should do everything in checkNode(), instead.

I refactored it a bit.

mdjermanovic · 2020-01-03T04:00:55Z

Thanks for the changes!

I think there is no need to keep the existing structure of the code if it doesn't provide value for the new behavior.

In particular, it feels like isRestrictedPath() and isRestrictedObject() are not very useful now. I believe that if we add this simple if at the beginning of reportPath() in the actual version of this PR:

function reportPath(importSource, importNames, node) {
    if (!restrictedPathMessages[importSource]) {
        return;
    }
    // ....
}

then we could rename reportPath(), call it directly and completely remove isRestrictedPath() and isRestrictedObject() functions from the code. Thoughts?

My idea was actually to simplify the code and do most of the logic in checkNode(): in short, iterate over specifiers and report them right away (including *). Your a bit more complex approach with the map is also fine for me 👍

lib/rules/no-restricted-imports.js

fanich37 · 2020-01-03T22:23:38Z

Thanks for the changes!

I think there is no need to keep the existing structure of the code if it doesn't provide value for the new behavior.

In particular, it feels like isRestrictedPath() and isRestrictedObject() are not very useful now. I believe that if we add this simple if at the beginning of reportPath() in the actual version of this PR:
function reportPath(importSource, importNames, node) {
    if (!restrictedPathMessages[importSource]) {
        return;
    }
    // ....
}
then we could rename reportPath(), call it directly and completely remove isRestrictedPath() and isRestrictedObject() functions from the code. Thoughts?

My idea was actually to simplify the code and do most of the logic in checkNode(): in short, iterate over specifiers and report them right away (including *). Your a bit more complex approach with the map is also fine for me 👍

Seems like a good idea! In this case I think that isRestrictedForEverythingImported and reportPathForEverythingImported are also useless. * could be reported in reportPath.
One thing I doubt about is reporting warnings in the specifiers loop. The clause to check whether report warning or not become quite big and difficult to reason about.

mdjermanovic · 2020-01-03T23:45:14Z

Seems like a good idea! In this case I think that isRestrictedForEverythingImported and reportPathForEverythingImported are also useless. * could be reported in reportPath.

I agree if it seems that the code would become easier to follow.

One thing I doubt about is reporting warnings in the specifiers loop. The clause to check whether report warning or not become quite big and difficult to reason about.

Agreed, making the importNames map first (as it's implemented) seems like a better idea and a nice separation of logic 👍

mdjermanovic

Thanks! I left a couple of notes just about naming, and one detail that I believe should be fixed.

lib/rules/no-restricted-imports.js

mdjermanovic

This looks very good! I just found a couple of minor details and I think there is one possible regression.

lib/rules/no-restricted-imports.js

tests/lib/rules/no-restricted-imports.js

fanich37 · 2020-01-06T21:16:29Z

This looks very good! I just found a couple of minor details and I think there is one possible regression.

Thank you for review and suggestions. And sorry for the long journey with this PR :)

mdjermanovic · 2020-01-07T00:14:01Z

Thanks for making all the changes and quickly addressing all concerns! It turned out that fixing this issue requires a quite big diff in the code.

As for the title of this PR, it might be safe to mark this as a semver-minor change (Update: instead of Fix:), because it can produce more warnings in some cases, caused by locations of eslint-disable-* comments as we are now changing reported locations. Also, 'handle multiple errors' maybe doesn't quite describe this change.

Thoughts about something like this?

Update: fix no-restricted-imports importNames reporting (fixes #12282)

mdjermanovic · 2020-01-07T02:46:42Z

About edge cases where the same name is imported multiple times in the same declaration, such as these:

import { a, a as b } from "mod";

export { x as y, x as z } from "mod";

import foo, { default as bar } from "mod";

It might be correct to report all specifiers for a restricted name instead of just the last one, if it doesn't add too much complexity to the code. Maybe we could store arrays in importNames (e.g., find out name in the if-else-ifs, then either .set(name, [specifierData]) or push to existing). or do something else if this doesn't seem like a good idea.

It also seems that the configuration allows duplicates in importNames, but that should be a separate issue (it would be better to throw configuration error than to account for this in the code). It's okay for now to report the same specifier multiple times when the rule is misconfigured in such way.

fanich37 · 2020-01-07T16:38:38Z

Thanks for making all the changes and quickly addressing all concerns! It turned out that fixing this issue requires a quite big diff in the code.

As for the title of this PR, it might be safe to mark this as a semver-minor change (Update: instead of Fix:), because it can produce more warnings in some cases, caused by locations of eslint-disable-* comments as we are now changing reported locations. Also, 'handle multiple errors' maybe doesn't quite describe this change.

Thoughts about something like this?
Update: fix no-restricted-imports importNames reporting (fixes #12282)

Sounds good!

mdjermanovic · 2020-01-07T21:06:43Z

Changed the title to Update:...

Regarding the cases where the same name is imported multiple times, I think it's fine to report just 1 specifier for now. Maybe we could change that later in a separate PR, if it looks useful.

tests/lib/rules/no-restricted-imports.js

fanich37 · 2020-01-07T23:02:53Z

About edge cases where the same name is imported multiple times in the same declaration, such as these:
import { a, a as b } from "mod";

export { x as y, x as z } from "mod";

import foo, { default as bar } from "mod";
It might be correct to report all specifiers for a restricted name instead of just the last one, if it doesn't add too much complexity to the code. Maybe we could store arrays in importNames (e.g., find out name in the if-else-ifs, then either .set(name, [specifierData]) or push to existing). or do something else if this doesn't seem like a good idea.

It also seems that the configuration allows duplicates in importNames, but that should be a separate issue (it would be better to throw configuration error than to account for this in the code). It's okay for now to report the same specifier multiple times when the rule is misconfigured in such way.

The problem here is the clause if (specifier.imported) ... else if (specifier.local). Since if specifier has imported property we never get into local branch of the clause...

mdjermanovic · 2020-01-07T23:39:58Z

The problem here is the clause if (specifier.imported) ... else if (specifier.local). Since if specifier has imported property we never get into local branch of the clause...

I don't think it would be a problem. If there is imported, it really shouldn't take local.

In the if-else-if chain, imported is for specifiers that appear in import declarations, while the
local is (probably unintuitive) for those in export declarations.

In the above examples:

import { a, a as b } from "mod";

export { x as y, x as z } from "mod";

import has 2 specifiers, both have a as imported.

export has also 2 specifiers, both have x as local, and nothing as imported (y and z are exported).

AST explorer link with the examples

What I was thinking is, for example in the first case if a is a restricted name from mod, that it might be correct to report 2 errors. One error with the location of the first a specifier, the other with the location of the a as b specifier. At the moment, a as b specifier will overwrite already set a location in the map. A solution might be to have [{ loc }, { loc } ...] in the map instead of just a single { loc }.

mdjermanovic

This looks great, thanks for the fix and the refactoring!

fanich37 · 2020-01-10T06:18:38Z

This looks great, thanks for the fix and the refactoring!

Sorry for the delay. It's just new year holidays that is over and I've got a big task at work :) But I would like to add your proposal about duplicate import names. I think I could add it till Monday.

And also don't you think it would be better to put all logic from checkPathAndReport into checkNode, checkPathAndReport rename back into reportPath. I just think it's a bit weird we have two check... functions in a rule? 😕

mdjermanovic · 2020-01-11T01:22:18Z

Sorry for the delay. It's just new year holidays that is over and I've got a big task at work :) But I would like to add your proposal about duplicate import names. I think I could add it till Monday.

That would be great, thanks!

And also don't you think it would be better to put all logic from checkPathAndReport into checkNode, checkPathAndReport rename back into reportPath.

Something like my initial idea? I'm not sure about that anymore, the way it's implemented now looks very nice (i.e. deal with different kinds of imports/exports first to get the importNames map and then do the main logic on a simplified input).

Also, it looks like the actual version in this PR might make it easier to add some new things in the future, like to fix #12737 and support #12629. At least I think so, could be wrong. If you think that the other approach might be better for future maintenance and easier to follow, I'm fine with both.

lib/rules/no-restricted-imports.js

mdjermanovic

Thanks, looks very good! Just two small details.

lib/rules/no-restricted-imports.js

mdjermanovic

LGTM, thanks!

Both rule and the tests look great 👍

kaicataldo · 2020-01-16T01:14:07Z

lib/rules/no-restricted-imports.js

@@ -95,6 +99,11 @@ module.exports = {
        const restrictedPaths = (isPathAndPatternsObject ? options[0].paths : context.options) || [];
        const restrictedPatterns = (isPathAndPatternsObject ? options[0].patterns : []) || [];

+        // if no imports are restricted we don"t need to check
+        if (Object.keys(restrictedPaths).length === 0 && restrictedPatterns.length === 0) {


kaicataldo

LGTM. Thanks for contributing to ESLint!

fanich37 · 2020-01-16T07:26:52Z

Thanks, looks very good! Just two small details.

Thank you for your review and suggestions 👍

mdjermanovic · 2020-01-17T00:14:52Z

@fanich37 thanks again for the really good work and I hope to see more PRs from you in the future! 👍

…#12282) (eslint#12711) * Process importNames * Update test cases * Fix rebase issue * Update importNames logic * Remove useless funcs, update tests * Fix naming, fix everything imported w/o importNames * Fix typo, fix specifier clause, fix rebase issue * Process importNames with the same name * Clean code in receiving specifier data, remove debug * Fix type def, add empty name check, replace concat with push

fanich37 added 2 commits December 25, 2019 17:08

Process importNames

f597ed4

Update test cases

9a09127

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Dec 25, 2019

fanich37 changed the title ~~No restricted imports multiple error (#12282)~~ Fix: handle multiple errors in no-restricted-imports (fixes #12282) Dec 25, 2019

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Dec 25, 2019

mdjermanovic reviewed Dec 25, 2019

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

Fix rebase issue

41979a8

mdjermanovic requested changes Dec 28, 2019

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

Update importNames logic

21fbe20

mdjermanovic reviewed Jan 3, 2020

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

mdjermanovic mentioned this pull request Jan 4, 2020

no-restricted-imports doesn't restrict export * when importNames are configured #12737

Closed

Remove useless funcs, update tests

47a9dbe

mdjermanovic requested changes Jan 5, 2020

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

Fix naming, fix everything imported w/o importNames

347d326

mdjermanovic requested changes Jan 6, 2020

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

tests/lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

Fix typo, fix specifier clause, fix rebase issue

1b720c4

fanich37 requested a review from mdjermanovic January 7, 2020 14:12

mdjermanovic changed the title ~~Fix: handle multiple errors in no-restricted-imports (fixes #12282)~~ Update: fix no-restricted-imports importNames reporting (fixes #12282) Jan 7, 2020

fanich37 commented Jan 7, 2020

View reviewed changes

tests/lib/rules/no-restricted-imports.js Show resolved Hide resolved

mdjermanovic reviewed Jan 7, 2020

View reviewed changes

tests/lib/rules/no-restricted-imports.js Show resolved Hide resolved

mdjermanovic approved these changes Jan 10, 2020

View reviewed changes

mdjermanovic self-requested a review January 10, 2020 22:55

Process importNames with the same name

56cc6b6

mdjermanovic reviewed Jan 11, 2020

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

mdjermanovic self-requested a review January 11, 2020 23:18

Clean code in receiving specifier data, remove debug

822059e

mdjermanovic requested changes Jan 12, 2020

View reviewed changes

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

lib/rules/no-restricted-imports.js Outdated Show resolved Hide resolved

Fix type def, add empty name check, replace concat with push

69c519d

mdjermanovic approved these changes Jan 15, 2020

View reviewed changes

kaicataldo reviewed Jan 16, 2020

View reviewed changes

kaicataldo approved these changes Jan 16, 2020

View reviewed changes

kaicataldo merged commit 68becbd into eslint:master Jan 16, 2020

fanich37 deleted the bugfix/nri-multiple-error branch January 16, 2020 07:26

eslint-deprecated bot locked and limited conversation to collaborators Jul 16, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: fix no-restricted-imports importNames reporting (fixes #12282) #12711

Update: fix no-restricted-imports importNames reporting (fixes #12282) #12711

fanich37 commented Dec 25, 2019

jsf-clabot commented Dec 25, 2019 •

edited

mdjermanovic left a comment

mdjermanovic left a comment

fanich37 commented Jan 2, 2020

mdjermanovic commented Jan 3, 2020

fanich37 commented Jan 3, 2020

mdjermanovic commented Jan 3, 2020

mdjermanovic left a comment

mdjermanovic left a comment

fanich37 commented Jan 6, 2020

mdjermanovic commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

fanich37 commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

fanich37 commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

mdjermanovic left a comment

fanich37 commented Jan 10, 2020

mdjermanovic commented Jan 11, 2020

mdjermanovic left a comment

mdjermanovic left a comment

kaicataldo Jan 16, 2020

kaicataldo left a comment

fanich37 commented Jan 16, 2020

mdjermanovic commented Jan 17, 2020

Update: fix no-restricted-imports importNames reporting (fixes #12282) #12711

Update: fix no-restricted-imports importNames reporting (fixes #12282) #12711

Conversation

fanich37 commented Dec 25, 2019

jsf-clabot commented Dec 25, 2019 • edited

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

fanich37 commented Jan 2, 2020

mdjermanovic commented Jan 3, 2020

fanich37 commented Jan 3, 2020

mdjermanovic commented Jan 3, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

fanich37 commented Jan 6, 2020

mdjermanovic commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

fanich37 commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

fanich37 commented Jan 7, 2020

mdjermanovic commented Jan 7, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

fanich37 commented Jan 10, 2020

mdjermanovic commented Jan 11, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

kaicataldo Jan 16, 2020

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

fanich37 commented Jan 16, 2020

mdjermanovic commented Jan 17, 2020

jsf-clabot commented Dec 25, 2019 •

edited