D-Scanner icon indicating copy to clipboard operation
D-Scanner copied to clipboard

New analysis: Useless assignment

Open andre2007 opened this issue 5 years ago • 10 comments

While trying to call a method/attribute of variable _fileSystem a segmentation fault will occur. There is a subtitle bug during the assignment in the constructor. I wonder whether this could be found by D-Scanner by throwing a warning for a self assignment _fileSystem = _fileSystem;?

class AppGenerator
{
    private IfFileSystem _fileSystem;

    this(IfFileSystem fileSystem)
    {
        _fileSystem = _fileSystem;
    }
}

andre2007 avatar Jul 09 '20 10:07 andre2007

important: it should catch

class AppGenerator
{
    private IfFileSystem fileSystem;

    this(IfFileSystem fielSystem)
    {
        this.fileSystem = fileSystem;
    }
}

WebFreak001 avatar Jul 10 '20 06:07 WebFreak001

To really work properly this would require D-Scanner to be a LOT more intelligent than it currently is.

Hackerpilot avatar Jul 14 '20 21:07 Hackerpilot

doesn't D-Scanner already depend on dsymbol which would be most of the logic?

WebFreak001 avatar Jul 15 '20 06:07 WebFreak001

There are a few ways to go about this.

  1. Check to see if the AST of the left side is the same as the right. This is really easy and we can just look at the if-else-same check and use the same approach here.
  2. Use DSymbol to see if the left and right expressions resolve to the same symbol. This is a bit slower but will correctly handle some uses of alias and this..
  3. Do some magic that will correctly handle things like this.tupleof[0] = firstField; I'm not sure how to handle this best. I think that this will require an enhancement to DSymbol, as I don't think that it currently supports resolving symbols through tupleof.

Hackerpilot avatar Jul 16 '20 09:07 Hackerpilot

I would prefer proposal 2. It looks reasonable. If really needed, proposal 3 could be added in future on top.

andre2007 avatar Jul 16 '20 11:07 andre2007

Proposal 2 also wouldn't be that slow, it would just need to be checked if the identifier on the left side is the same as on the right side. I think supporting aliases or other confusing things would be overkill.

WebFreak001 avatar Jul 16 '20 12:07 WebFreak001

I started thinking about implementing this and realized that we'd need a way to provide import paths so that the DSymbol code can figure things out using imported modules. Then I found out that we already have that, but that it's not documented in the output of dscanner --help. https://github.com/dlang-community/D-Scanner/pull/812

Hackerpilot avatar Jul 17 '20 01:07 Hackerpilot

There is also a pr to implement the check as dmd compiler warning https://github.com/dlang/dmd/pull/11553

andre2007 avatar Aug 13 '20 21:08 andre2007

It's probably a better idea to implement this in the compiler than in D-Scanner.

Hackerpilot avatar Aug 13 '20 22:08 Hackerpilot

There is also a pr to implement the check as dmd compiler warning dlang/dmd#11553

Self-assignment of members in constructors will be an error. Others a warning.

nordlow avatar Aug 14 '20 06:08 nordlow