Implement warnings for unused imports

[RazvanN7]

This currently works only for general imports when -w or -wi is passed. It will consider renamed or selective imports as being used even though the actual alias is never referenced (this falls into the category of unused variables, rather than unused imports and may be fixed in the future, provided that this PR is accepted).

The implementation modifies ScopeDsymbol to use a hashtable that links the import to the actual symbol for which the scope is imported. This makes the code nicer and even more efficient (O(n) to search for a scope that is being inserted is transformed into a O(1) hashtable lookup) at no memory cost. Therefore, I would argue that the first commit should be merged regardless of whether we accept warnings for unused params. The cost is that a field is added to every import to store whether the import was used or not, however, I think this is a negligible cost since programs don't have that many imports.

[dlang-bot]

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	24778	enhancement	Warning for unused imports

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#16878"

[RazvanN7]

cc @WalterBright

[RazvanN7]

Ok, seems like imports in template declarations will be always considered not used. Need to fix this.

[RazvanN7]

Ok, there are a couple of issues to be tackled here to fix the druntime failures:

• template declarations that contain imports(I think a sane solution is to simply skip analyzing the template declaration and just do the analysis on the template instance).
• template declarations that use symbols from an import from an upper level - with the machinery present in the compiler it is not possible to decide whether a template uses an import or not.
• version conditions don't currently work.
• static imports currently don't work for whatever implementation reason.
• imports that are at top-level but they are used only inside unittests (these should probably be moved inside the unittest).

[TurkeyMan]

I think the result of this work, would be for imports that are only used in some conditions (in versions, or in templates) to naturally migrate inside their respective conditional guards.

I agree that the only sensible thing to do with templates is to mark them as references upon instantiation. So global imports used only in an uninstantiated template should warn that they're unused.

What is the challenge? I would have imagined that this and also unused variable analysis might use exactly the same implementation; put a bool on all symbols, and mark it true when there is any reference. Anything that was not marked at the end of compilation is unused... is that not a reliable test for this?

[ibuclaw]

Also import.h

[ibuclaw]

Ok, there are a couple of issues to be tackled here to fix the druntime failures:

What about public imports?

[RazvanN7]

I think the result of this work, would be for imports that are only used in some conditions (in versions, or in templates) to naturally migrate inside their respective conditional guards.

This might be inconvenient if you have multiple templates using the same import. In addition, that might not be possible if, for example, you have a symbol being used in a template constraint.

What is the challenge?

The compiler codebase. Getting the 90% done is fairly easy, but the remaining 10% is hell. First I want to make sure I get imports right, then we can extend in subsequent PR to unused vars.

[RazvanN7]

What about public imports?

Currently they are treated as normal imports: if they're not being used in the module they are declared in they are flagged. Are you suggesting these should be exempted from the check?

[thewilsonator]

Yes, they are part of the public API for that module. Most package.d files will do this.

[RazvanN7]

Makes sense

[TurkeyMan]

I think the result of this work, would be for imports that are only used in some conditions (in versions, or in templates) to naturally migrate inside their respective conditional guards.

This might be inconvenient if you have multiple templates using the same import.

Nar, that seems perfectly fine to me; desirable even.

In addition, that might not be possible if, for example, you have a symbol being used in a template constraint.

That's an interesting case... can you mark mark symbols as having been used where they appear in constraint expressions?

I feel that makes sense; the constraint is not really a part of the instantiation, it's a predicate and it must be evaluated in all cases prior to instantiation. It's kind-of at a level outside the template, and it does make sense to mark symbol references in the constraint expression as 'used'.

Where the constraint expression may reference symbols inside a template evaluation, obviously those won't be caught this way, but that's actually good; they will be caught when the template is instantiated as usual. It's only the symbols that appear in the constraint expression directly that should be marked as 'used' this way... and I reckon that should be possible during an early semantic pass?

[RazvanN7]

I feel that makes sense; the constraint is not really a part of the instantiation, it's a predicate and it must be evaluated in all cases prior to instantiation

It all depends on how you view the instantiation. You are right that the template constraint must be evaluated before instantiation, but what the compiler does in this case is identical to what it does when it generates the template instance body: it needs to do type binding, semantic analysis (+ interpretation). So from that point of view I don't see how you can actually know what the symbols refer to without doing semantic analysis and you cannot do semantic analysis without having a concrete type. You could potentially do a mock-up search of the identifiers present in the template constraint, provided that you somehow get a grip on the scope of the template constraint, but I'm not sure what that would yield - might work, might not - I suppose I can give it a shot and see what are the results.

[TurkeyMan]

You could potentially do a mock-up search of the identifiers present in the template constraint, provided that you somehow get a grip on the scope of the template constraint, but I'm not sure what that would yield - might work, might not - I suppose I can give it a shot and see what are the results.

Yes, that's essentially what I mean. It might be enough to scan the expression for symbol references without full expression evaluation. Any symbols in the expression can be confidently marked as 'used' I think? If there are templates in the expression, it doesn't need to be evaluated/instantiated at this time, because any symbols they reference should be internal to the template, and they will be caught on instantiation as usual...? The template itself is used though, but you don't need to instantiate to know that.

[ibuclaw]

Wrt templates

template T(S)
{
    import maybe_used;
    static if (evals_true)
    {
         // ...
    }
    else if (uses_import)
    {
        // ...
    }
}

How does this fair when an import is used in conditionally compiled code?

[TurkeyMan]

The template seems irrelevant in this example, this situation can happen in any static-if series.
It kinda looks like static if condition expressions are the same as function constraint expressions as discussed...