[new rule] No global imports #390
Conversation
juanpcapurro
force-pushed
the
no-global-imports
branch
from
0593a48
to
7f3e410
Compare
| @@ -9,7 +9,7 @@ title: "comprehensive-interface | Solhint" | |||
|  | |||
|
|
|||
| ## Description | |||
| Check that all public or external functions are override. This is useful to make sure that the whole API is extracted in an interface. | |||
| Check that all public or external functions are override. This is iseful to make sure that the whole API is extracted in an interface. | |||
There was a problem hiding this comment.
that's from the documentation update, from #389 , which this PR depends on.
I wanted to get feedback as early as possible, but for that you and other reviewers should look at the changes introduced in the last two commits.
It seems that it wasn't obvious and I want to be respectful of your time so I'll mark this PR as a draft until #389 is merged, sorry for the mixup
There was a problem hiding this comment.
sorry... I was just going quickly through the changes, I saw this and just asked.... I'm not assigned full time to this project. And I'm also new to it. I did not approve the former pr because of that. Waiting a few days if fvitorio (the former maintainer) can check it as well... if not, I will do it.
Your contributions are very valuable, sorry if we can't keep up
juanpcapurro
force-pushed
the
no-global-imports
branch
from
7f3e410
to
f5fc951
Compare
|
|
||
| SourceUnit(node) { | ||
| node.children.forEach((it) => { | ||
| if (it.type === 'ImportDirective' && !it.symbolAliases) { |
There was a problem hiding this comment.
It would be better to use the ImportDirective visitor callback instead of iterating here. Not a big difference, but it's more idiomatic.
There was a problem hiding this comment.
Initially I had an issue by which the ImportDirective callback wasn't being called for the nodes not defining symbolAliases, which are the interesting ones. But I can't replicate it now 🙃 so it was probably something else.
pushed again using the ImportDirective callback as suggested.
There was a problem hiding this comment.
seems to me this can be resolved and pr approved
do you agree @fvictorio ?
juanpcapurro
force-pushed
the
no-global-imports
branch
from
f5fc951
to
b2cad15
Compare
| } | ||
|
|
||
| ImportDirective(node) { | ||
| if (node.type === 'ImportDirective' && !node.symbolAliases) { |
There was a problem hiding this comment.
No need to check the node's type, the ImportDirective callback is only called when an import node is visited.
| if (node.type === 'ImportDirective' && !node.symbolAliases) { | |
| if (!node.symbolAliases) { |
disallow imports that potentially pollute the namespace by importing every defined name of the imported file. ~when registering an ImportDirective method on the NoGlobalImportsChecker class, it's only called by imports that explicitly define a new Identifier, the only way I found to find global imports is to search for them in the SourceUnit nodes~ ^ was recommended against in review and when trying to replicate it to show the issue to fvictorio, I wasn't able to. So the cleaner way, using the ImportDirective visitor, will remain.
I haven't seen global imports on recent codebases, and they're [recommended against](https://docs.soliditylang.org/en/v0.8.17/layout-of-source-files.html#importing-other-source-files) in the solidity documentation, so I think it's reasonable to have the rule as recommended
juanpcapurro
force-pushed
the
no-global-imports
branch
from
b2cad15
to
f371bad
Compare
depends on #389
this is my first time near a syntax tree, so I'm looking forward to your feedback!
I also took the liberty of making it a recommended rule, but if there isn't consensus we could just drop the last commit
as always, see technical details in git history