dlang-community / D-Scanner

Swiss-army knife for D source code
Boost Software License 1.0
241 stars 80 forks source link

New analysis: Useless assignment #810

Open andre2007 opened 4 years ago

andre2007 commented 4 years ago

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;
    }
}
WebFreak001 commented 4 years ago

important: it should catch

class AppGenerator
{
    private IfFileSystem fileSystem;

    this(IfFileSystem fielSystem)
    {
        this.fileSystem = fileSystem;
    }
}
Hackerpilot commented 4 years ago

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

WebFreak001 commented 4 years ago

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

Hackerpilot commented 4 years ago

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.
andre2007 commented 4 years ago

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

WebFreak001 commented 4 years ago

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.

Hackerpilot commented 4 years ago

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

andre2007 commented 4 years ago

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

Hackerpilot commented 4 years ago

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

nordlow commented 4 years ago

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.