www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Unused import tool

reply RazvanN <razvan.nitu1305 gmail.com> writes:
Hello everyone,

I have started working on [a tool that identifies unused 
imports](https://github.com/RazvanN7/Unused-Import/tree/master). 
It uses dmd as a library.

The purpose is to have a working tool, but also to better 
understand what sort of information the frontend needs to output 
so that such tools are possible.

Up to this point, I managed to make it work with global imports 
(since dmdfe currently does not provide the scope hierarchy). The 
tool does output some false positives because semantic is 
destructive and some things like aliases or enums are substituted 
eagerly and you cannot trace their origin.

I have tested this tool with some dmd sources and have already 
identified [unused 
imports](https://github.com/dlang/dmd/pulls?q=is%3Apr+delete+unused+imports+author%3ARazvanN7+is%3Aopen)·

So, if you have projects with lots of global imports you could 
use it to at least narrow down the list of potential unused 
imports.

The tool is a work in progress, so if you have any ideas for 
improvement feel free to ping me.

Note that the import paths used are from my machine, so you will 
need to update those accordingly to the files imported in your 
project.

RazvanN
Aug 15 2023
next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 8/15/23 10:13, RazvanN wrote:

 
 I have tested this tool with some dmd sources and have already 
 identified [unused 
 imports](https://github.com/dlang/dmd/pulls?q=is%3Apr+delete+unused+imports+author%3ARazvanN7+is%3Aopen)·


Not a big complaint, just something of which to be aware: Removing 
imports that are actually unused but can be relevant to the module will 
break forks on rebase. E.g., the removal of `import dmd.tokens` from 
`attrib.d` broke my implementation of UnpackDeclaration in the 
`tuple-syntax` branch.
Aug 15 2023
parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Tuesday, 15 August 2023 at 09:28:53 UTC, Timon Gehr wrote:

 On 8/15/23 10:13, RazvanN wrote:

 
 I have tested this tool with some dmd sources and have already 
 identified [unused 
 imports](https://github.com/dlang/dmd/pulls?q=is%3Apr+delete+unused+imports+author%3ARazvanN7+is%3Aopen)·


 Not a big complaint, just something of which to be aware: 
 Removing imports that are actually unused but can be relevant 
 to the module will break forks on rebase. E.g., the removal of 
 `import dmd.tokens` from `attrib.d` broke my implementation of 
 UnpackDeclaration in the `tuple-syntax` branch.


There are other situations where this is problematic: versioned 
statements, debug code etc. However, I would argue that useless 
imports for various builds of the compiler should be removed as 
they come with a compilation penalty. The solution to this is to 
make the imports more localized (e.g. put the import in the 
versioned block that is used - although this is not ideal if 2 
multiple versions use the same import).

As for your specific case, I think it can be seen as removing a 
useless import that is then re-added once the patch is pushed to 
upstream. This is no different from any other interface 
modification: if someone adds a parameter to a function that you 
are using in your fork, then that will also break your code. 
Anyway, thanks for the heads up!

RazvanN
Aug 15 2023
parent reply Walter Bright <newshound2 digitalmars.com> writes:
I ask that `import core.stdc.stdio;` stop being regularly deleted, as I 
regularly add it back in. dmd uses printf all over the place for debug
information.
Aug 18 2023
next sibling parent reply "Richard (Rikki) Andrew Cattermole" <richard cattermole.co.nz> writes:
On 19/08/2023 5:00 AM, Walter Bright wrote:

 I ask that `import core.stdc.stdio;` stop being regularly deleted, as I 
 regularly add it back in. dmd uses printf all over the place for debug 
 information.


This is sounding an awful lot like the import should be done where it is 
used and a dedicated log functions should be implemented.

I quite often do something like:

```d
version(none) {
	debug {
		try {
			import std.stdio;
			writeln("...");
			stdout.flush;
		} catch (Exception) {
		}
	}
}
```

I.e.

https://github.com/Project-Sidero/basic_memory/blob/main/source/sidero/base/text/internal/builder/operations.d#L1199

Also add random debug blocks to do extra verification:

https://github.com/Project-Sidero/basic_memory/blob/main/source/sidero/base/text/internal/builder/operations.d#L783

Both ``version(none)`` and ``debug`` blocks are absolutely amazing tools 
for dealing with debug only code that shouldn't see production!

Very helpful for when I come back to complex code after many months or 
years and hitting segfaults if you catch my drift ;)
Aug 18 2023
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/18/2023 10:11 AM, Richard (Rikki) Andrew Cattermole wrote:

 I quite often do something like:
 
 ```d
 version(none) {
      debug {
          try {
              import std.stdio;
              writeln("...");
              stdout.flush;
          } catch (Exception) {
          }
      }
 }
 ```


In the dmd source code, there are a lot of commented out printf's. There would 
be a lot of clutter if an import statement had to be added for each of them.
Aug 20 2023
parent reply FeepingCreature <feepingcreature gmail.com> writes:
On Sunday, 20 August 2023 at 16:17:34 UTC, Walter Bright wrote:

 In the dmd source code, there are a lot of commented out 
 printf's. There would be a lot of clutter if an import 
 statement had to be added for each of them.


To be fair, these are terrible: like half of them don't work, 
because the variables involved have been renamed or removed 
entirely. Something like `debugPrintf("...", args)` that compiled 
to a no-op in release mode would be a lot better, and also avoid 
the "unused import" problem, no? Maybe even 
`debugPrintf!"semantic2"("...", args)`, or `alias debugPrintf = 
debugPrintf!"semantic2"` at the top.
Aug 21 2023
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/21/2023 3:45 AM, FeepingCreature wrote:

 To be fair, these are terrible: like half of them don't work, because the 
 variables involved have been renamed or removed entirely.


I run across that now and then, and fix them. It's not been a significant
problem.


 Something like 
 `debugPrintf("...", args)` that compiled to a no-op in release mode would be a 
 lot better, and also avoid the "unused import" problem, no? Maybe even 
 `debugPrintf!"semantic2"("...", args)`, or `alias debugPrintf = 
 debugPrintf!"semantic2"` at the top.


I did things like that for a while, and finally reverted to the stupid simple 
approach of printf.
Aug 21 2023
parent FeepingCreature <feepingcreature gmail.com> writes:
On Tuesday, 22 August 2023 at 01:54:48 UTC, Walter Bright wrote:

 On 8/21/2023 3:45 AM, FeepingCreature wrote:

 To be fair, these are terrible: like half of them don't work, 
 because the variables involved have been renamed or removed 
 entirely.


 I run across that now and then, and fix them. It's not been a 
 significant problem.


Man, you wrote the thing, it's not surprising it isn't a problem 
for you! :)

If `sc` is gone, and now there's some other classes you haven't 
heard of, that may be subclasses of scopes or contain various 
numbers of scopes, good luck guessing. `printf` isn't just for 
debugging, it's also for learning how the compiler is supposed to 
do things; that gets more difficult if you have to guess which 
member is the right one to pay attention to.

Idk, simplification is good, but maybe I'd just have chucked a 
global `alias printf = printfNoOp;` in or something.
Aug 22 2023
prev sibling parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Friday, 18 August 2023 at 17:00:33 UTC, Walter Bright wrote:

 I ask that `import core.stdc.stdio;` stop being regularly 
 deleted, as I regularly add it back in. dmd uses printf all 
 over the place for debug information.


I haven't deleted any core.stdc.stdio imports in the PRs that 
have been enabled by the tool (even though the tool correctly 
reported those as being unused).
Aug 20 2023
parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/20/2023 4:43 AM, RazvanN wrote:

 I haven't deleted any core.stdc.stdio imports in the PRs that have been
enabled 
 by the tool (even though the tool correctly reported those as being unused).


Thank you.
Aug 20 2023
prev sibling next sibling parent cc <cc nevernet.com> writes:
On Tuesday, 15 August 2023 at 08:13:04 UTC, RazvanN wrote:

 I have started working on [a tool that identifies unused 
 imports](https://github.com/RazvanN7/Unused-Import/tree/master). It uses dmd
as a library.


Not sure how relevant this is to your methods, but something I 
used to stumble into occasionally:

```d
import std.format;
import std.range.primitives;
struct S {
	void toString(W)(ref W writer) if (isOutputRange!(W, char)) {
		writer.formattedWrite("ABC");
	}
}
void main() {
	S().writeln;
}
```
Output: `ABC`


```d
import std.format;
//import std.range.primitives;
struct S {
	void toString(W)(ref W writer) if (isOutputRange!(W, char)) {
		writer.formattedWrite("ABC");
	}
}
void main() {
	S().writeln;
}
```
Output: `S()`

The latter still compiles successfully and does not issue an 
error for the undefined template `isOutputRange`.  This is an 
issue with std.format.internal's `hasToString` I believe, since, 
because there is no toString that would successfully pass 
`__traits(compiles)` (due to the undefined symbol), it never 
actually attempts to *call* the toString, therefore the template 
isn't compiled at all and thus no bark for the problem. But worth 
mentioning IMO as an example of a process that fails to identify 
undefined symbols when an import is missing.
Aug 15 2023
prev sibling parent reply Basile B. <b2.temp gmx.com> writes:
On Tuesday, 15 August 2023 at 08:13:04 UTC, RazvanN wrote:

 Hello everyone,

 I have started working on [a tool that identifies unused 
 imports](https://github.com/RazvanN7/Unused-Import/tree/master). It uses dmd
as a library.

 The purpose is to have a working tool, but also to better 
 understand what sort of information the frontend needs to 
 output so that such tools are possible.

 Up to this point, I managed to make it work with global imports 
 (since dmdfe currently does not provide the scope hierarchy).


You can handle nested import declarations without the scope. The 
trick would be to use a single visitor and a 2D import list, 
i.e`ImportInfo[][] imports;`. The first dim matches to a scope, 
so when you enter a scope (BlockStmt, AggrDecl, etc) you push the 
local imports, and when you leave the scope, you check and pop. 
Styx 
[Example](https://gitlab.com/styx-lang/styx/-/blob/master/src/styx/lint.sx?r
f_type=heads#L247), (which has same semantics as D about out-of-order decls).


 The tool does output some false positives because semantic is 
 destructive and some things like aliases or enums are 
 substituted eagerly and you cannot trace their origin.


This is why I think that such a tool should be integrated to the 
compiler.

The only way (I know of today) to detect accurately whether an 
alias is used is to put of flag in the ast node matching to 
imports. For dmd that flag would be set in `Import.search()`.


 I have tested this tool with some dmd sources and have already 
 identified [unused 
 imports](https://github.com/dlang/dmd/pulls?q=is%3Apr+delete+unused+imports+author%3ARazvanN7+is%3Aopen)·

 So, if you have projects with lots of global imports you could 
 use it to at least narrow down the list of potential unused 
 imports.

 The tool is a work in progress, so if you have any ideas for 
 improvement feel free to ping me.

 Note that the import paths used are from my machine, so you 
 will need to update those accordingly to the files imported in 
 your project.


Again another reason to integrate the tool to the compiler. There 
would be no need to write a driver, just a new option, let's say 
"--check-unused-import", and "basta".

Also I see no use case where the tool would be used with 
different import paths than those passed to the compiler.


 RazvanN
Aug 15 2023
next sibling parent "Richard (Rikki) Andrew Cattermole" <richard cattermole.co.nz> writes:
On 16/08/2023 5:26 PM, Basile B. wrote:

 Again another reason to integrate the tool to the compiler. There would 
 be no need to write a driver, just a new option, let's say 
 "--check-unused-import", and "basta".


It would be part of dscanner that is built on dmd-fe.

Dscanner in recent versions is getting the ability to 'fix' code like 
dfix did.

So not only could dscanner check for unused imports, but it could remove 
them. Or even ideally add imports.
Aug 15 2023
prev sibling next sibling parent reply RazvanN <razvan.nitu1305 gmail.com> writes:
On Wednesday, 16 August 2023 at 05:26:11 UTC, Basile B. wrote:


 You can handle nested import declarations without the scope. 
 The trick would be to use a single visitor and a 2D import 
 list, i.e`ImportInfo[][] imports;`. The first dim matches to a 
 scope, so when you enter a scope (BlockStmt, AggrDecl, etc) you 
 push the local imports, and when you leave the scope, you check 
 and pop. Styx 
 [Example](https://gitlab.com/styx-lang/styx/-/blob/master/src/styx/lint.sx?r
f_type=heads#L247), (which has same semantics as D about out-of-order decls).


Indeed, there are solutions to this problem. Another one is to 
treat scope as dscanner does now (the scope is just an integer 
which is incremented and decremented whenever a scope is 
entered/left), however, I was hoping to leverage most of the 
existing logic in the compiler.


 This is why I think that such a tool should be integrated to 
 the compiler.


I am now looking for a way to integrate this in the compiler to 
better understand what the compiler would need to expose in order 
to properly implement this. I thought it would be easy, 
unfortunately, when dmd does symbol resolution, the search 
methods work on a list of imported scopes (e.g. modules or 
mixins) and currently there is no way to tie the imported scope 
to a specific import declaration/statement. DMD's code could be 
altered to store the import instead of the module, however, 
Walter has made it clear that such linting behavior will not be 
accepted in dmd.

RazvanN
Aug 16 2023
parent Basile B. <b2.temp gmx.com> writes:
On Wednesday, 16 August 2023 at 07:27:34 UTC, RazvanN wrote:

 On Wednesday, 16 August 2023 at 05:26:11 UTC, Basile B. wrote:


 You can handle nested import declarations without the scope. 
 [..]


 Walter has made it clear that such linting behavior will not be 
 accepted in dmd


I know, but he should make an exception for unused imports. 
That's really a check that is tied to which and how sources are 
compiled. I find absurd to have to write a driver that will 
handle the exact same arguments, while that would just work if 
done in the compiler, to be frank.

It's another story when it's about linting something localized. 
But in the case we're talking about, well, you've get my point.
Aug 16 2023
prev sibling parent reply Basile B. <b2.temp gmx.com> writes:
On Wednesday, 16 August 2023 at 05:26:11 UTC, Basile B. wrote:

 On Tuesday, 15 August 2023 at 08:13:04 UTC, RazvanN wrote:

 Hello everyone,

 I have started working on [a tool that identifies unused 
 imports](https://github.com/RazvanN7/Unused-Import/tree/master). It uses dmd
as a library.

 The purpose is to have a working tool, but also to better 
 understand what sort of information the frontend needs to 
 output so that such tools are possible.

 Up to this point, I managed to make it work with global 
 imports (since dmdfe currently does not provide the scope 
 hierarchy).


 You can handle nested import declarations without the scope. 
 The trick would be to use a single visitor and a 2D import 
 list, i.e`ImportInfo[][] imports;`. The first dim matches to a 
 scope, so when you enter a scope (BlockStmt, AggrDecl, etc) you 
 push the local imports, and when you leave the scope, you check 
 and pop. Styx 
 [Example](https://gitlab.com/styx-lang/styx/-/blob/master/src/styx/lint.sx?r
f_type=heads#L247), (which has same semantics as D about out-of-order decls).


To be clear the styx version had the same problem as those you 
mention, until [that 
commit](https://gitlab.com/styx-lang/styx/-/commit/b890f94f78e05c244be67a
43e8d8d013a018048). The flag mentioned previously, and that is set
[here](https://gitlab.com/styx-lang/styx/-/commit/b890f94f78e05c244be67a743e8d8d013a018048#bd524f0aa3bc570bcb844ae4f453b
f2a32db4ae_354_355) (implementation of search is a bit different...), did not
cause any slowdown.

To conclude, I dont see how such a tool could work correctly 
without being integrated directly in the compiler.
Aug 16 2023
parent Basile B. <b2.temp gmx.com> writes:
On Wednesday, 16 August 2023 at 17:17:22 UTC, Basile B. wrote:

 On Wednesday, 16 August 2023 at 05:26:11 UTC, Basile B. wrote:

 [...]


 To be clear the styx version had the same problem as those you 
 mention, until [that 
 commit](https://gitlab.com/styx-lang/styx/-/commit/b890f94f78e05c244be67a
43e8d8d013a018048). The flag mentioned previously, and that is set
[here](https://gitlab.com/styx-lang/styx/-/commit/b890f94f78e05c244be67a743e8d8d013a018048#bd524f0aa3bc570bcb844ae4f453b
f2a32db4ae_354_355) (implementation of search is a bit different...), did not
cause any slowdown.

 To conclude, I dont see how such a tool could work correctly 
 without being integrated directly in the compiler.


Otherwise another problem you may encounter is related to dot 
chains. only the LHS needs to be checked. The RHS might be 
resolved from the module where the LHS was found ;)
Aug 16 2023