WIP: Diagnostics: Equal sub-expressions and sub-statements [skip ci]

[nordlow]

Diagnostics Experimentation Playground

Inspired by warning flags in GCC, Clang, and CppCheck. I see the need for helper functions. So I join these in a PR mergeable with master for now so we can experiment with these all at once in existing projects. Helper functions are

• equality/equivalence check of ASTNode (trees) currently implemented in equalsExp

By evaluating checks in an iterative process we can incrementally adjust sensible rules for when to exclude diagnostics, for instance, in assert statements, unittests and (unittest) calls of overloaded aggregate logical, bitwise and assignment operators. In the hope that these checks, in the long run, can be enabled by default. This because false positives are more harmful to the developer experience than false negative.

N-ary Expressions giving No-Op for Equal Sub-Expressions

Detects mistakes such as

int x;
if (x && x) {}
const _ = true ? x : x;

After having tested these on dmd, druntime and phobos I've decided to disable this diagnostics for enums because (generic) unittests may test behaviour of overridden (binary) operators using constants inside assert statements. It might be better to exclude this check for overridden operators and for any expression inside a unittest.

Check for equality of sub-expressions must be maximally fast so we can have it by default. Therefore I'm only using the isXXX members of Expression for now to avoid any virtual member calls such as equals(const RootObject). Later on equalsExp should be merged with equals or moved into a final Expression.equals(const Expression).

Self-Assignment of Variables

• Turn self assignment of aggregate member variables inside all members functions into errors.
• When warnings are enable other assignments issue deprecations except when the type has either an opAssign, postblit, destructor or a copy construtcor.
• The motivation here is to detect more operations that have no side-effect similar to discarded return values from pure functions as in

int f() { return 42; }
@safe pure unittest { f(); }

Comments:

This diagnostics is especially important in constructors when one accidentally write

this(T)(T someMember)
{
   someMember = someMember;
}

when one really meant

this(T)(T someMember)
{
   this.someMember = someMember;
}

For reference, the C code

int x = 0;
x = x;

triggers a similar diagnostics

warning: explicitly assigning value of variable of type 'int' to itself

when compiled with clang-10.

Furthermore, in Clang, its worth noticing that in cases such as

class C
{
    C(int x)
    {
        x = x;
        _x = _x;
    }
    int _x;
};

diagnoses as warnings only for both the normal variable and member variable cases:

warning: explicitly assigning value of variable of type 'int' to itself
warning: assigning field to itself

[dlang-bot]

Thanks for your pull request and interest in making D better, @nordlow! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#11553"

[wilzbach]

Needs a test case and/or updated test suite with AUTO_UPDATE=1

Edit: I overlooked that you mentioned this in your PR description. Sorry.

[Geod24]

I don't think flat-out disallowing self-assignment is very sensible. The underlying assumption is that those are no-op and mistakes, but they don't necessarily are. It could be generic code, or it could be a variable with a side effect on opAssign / copy constructor. In general, we try to not provide warnings or error for things that could be errors. Because false positives give a terrible UX. There are few exceptions, e.g. switch-case fallthrough, but this is mostly to keep compatibility with C code: if we were to design from the ground up, I would suggest we simply don't put fall-through in the language in the first place. Since we can't do that, we effectively removed it, but forced the user to use a certain construct.

Also, comparing the ident is just not going to work. See the current failure on:

dmd/src/dmd/iasmgcc.d

Lines 403 to 404 in e9cc3dd

	scope tint32 = new TypeBasic(ASTCodegen.Tint32);
	ASTCodegen.Type.tint32 = tint32;

Since the main issue is in constructor, I would suggest to only disallow self-assignment of fields within constructors, and leave the rest be.

[MoonlightSentinel]

It could be generic code

This change would break the following example which tests whether t is assignable.

__traits(compiles, t = t)

Not the best way to do it but it's probably out there.

[drathier]

FWIW, "self-assignment of variables is deprecated" is a horrible error message. It does tell me why or what I can do about it.

[WalterBright]

This is a more intrusive change than it appears:

int x;
void test() {
    int x = x;
}

This is not self-assignment. the second declaration of x does not exist until after it is initialized, so the outer scope x is unambiguously selected.

This kind of change absolutely needs a precise definition of what semantics it has before we can evaluate it.

[nordlow]

This is a more intrusive change than it appears:

int x;
void test() {
    int x = x;
}

This is not self-assignment. the second declaration of x does not exist until after it is initialized, so the outer scope x is unambiguously selected.

This kind of change absolutely needs a precise definition of what semantics it has before we can evaluate it.

This case is not triggered in my latest commit as I'm comparing the lhs-var and rhs-var for equivalence. I'm not using the var.ident anymore. That was a big mistake from my side. Thanks @ibuclaw for pointing that out.

Currently

pure unittest
{
    int x;
    x = x;                      // diagnose

    int y;
    y = x;

    int* xp;
    xp = xp;                    // diagnose

    *(&x) = *(&x);              // should diagnose
}

int x;
void test() @safe nothrow @nogc
{
    int x = x;                  // shouldn't this give a shadowing warning?
}

is diagnosed as

self_assign.d(4,7): Warning: assignment of `x` to itself has no side effect
self_assign.d(10,8): Warning: assignment of `xp` to itself has no side effect

BTW, shouldn't the declaration of int x = x; trigger a variable shadowing error, @WalterBright?

[nordlow]

__traits(compiles, t = t)

This

static assert(__traits(compiles, { int t; t = t; }));

passes with last commit and with exp.warning instead of exp.deprecation.

[nordlow]

FWIW, "self-assignment of variables is deprecated" is a horrible error message. It does tell me why or what I can do about it.

How about

Warning: assignment of `x` to itself has no side effect

?

[WalterBright]

Still need some sort of specification of what the intended behavior is.

[Geod24]

Still need some sort of specification of what the intended behavior is.

I strongly second this. So far the cons of this far out-weight the pros IMO. Limiting it to constructors (perhaps even methods) would be one way to improve this, because that's where the error is most common.

[drathier]

How common is this mistake in practice?

[nordlow]

How common is this mistake in practice?

It's happened to me a handful number of times.

[nordlow]

Now correctly analyses this

struct S
{
pure nothrow:
    this(this) { count += 1;}   // posblit
    int count;
}
pure unittest
{
    S s;
    s = s;

    int x;
    x = x;                      // diagnose

    int y;
    y = x;

    int* xp;
    xp = xp;                    // diagnose

    *xp = *xp;                  // diagnose

    *&x = *&x;              // diagnose

    static assert(__traits(compiles, { int t; t = t; }));
}

int x;
void test() @safe nothrow @nogc
{
    int x = x;                  // shouldn't this give a shadowing warning?
}

[nordlow]

Needs a test case and/or updated test suite with AUTO_UPDATE=1

Edit: I overlooked that you mentioned this in your PR description. Sorry.

Test has been added. No ready for review yet, though.

[andre2007]

This new check is super useful. I was several time facing segmentation faults. It was always caused by self assignments in constructors (when assignment of an argument was meant).

[nordlow]

This new check is super useful. I was several time facing segmentation faults. It was always caused by self assignments in constructors (when assignment of an argument was meant).

Those are motivating words. Thanks. I'm planning more checks.

[nordlow]

New diagnostics now finds the following two suspicious lines that breaks the autotester:

Line 1530 in paranoia.d containing

X = X;

triggers error

test/runnable/extra-files/paranoia.d(1530,5): Error: assignment of member `this.X` from itself

Line 6245 in interpret3.d containing

coord = coord;

triggers error

test/compilable/interpret3.d(6245,15): Error: construction of member `this.coord` from itself

Are these two statements intentional or bugs?

Is it ok to modify them to silence the warnings?

@wilzbach?

[nordlow]

For reference, the code

int x = 0;
x = x;

triggers a similar diagnostics

warning: explicitly assigning value of variable of type 'int' to itself

for Clang.

Should I stick to my diagnostics naming "from itself"?

Furthermore, in Clang, its worth noticing that

class C
{
    C(int x)
    {
        x = x;
        _x = _x;
    }
    int _x;
};

diagnoses as warnings only:

warning: explicitly assigning value of variable of type 'int' to itself
warning: assigning field to itself

[nordlow]

Is it ok for

int g_x;
alias g_y = g_x;
g_y = g_y;

to diagnose as

diag_self_assign.d(2): Warning: assignment of `g_x` from itself has no side effect

or is it better to print

diag_self_assign.d(2): Warning: assignment of `g_y` from itself has no side effect

? What's the preferred way of handling aliases in diagnostics?

[wilzbach]

Are these two statements intentional or bugs?
test/compilable/interpret3.d(6245,15): Error: construction of member this.coord from itself

This looks like a bug to me (showing the merits of your PR).

Is it ok to modify them to silence the warnings?

Yes in general it's ok.
Reviewers will always be able to see it in the diff and judge.
In fact, it's better, because then there's context to the lines and a few clicks to open the respective file can be saved.

test/runnable/extra-files/paranoia.d(1530,5): Error: assignment of member this.X from itself

AFAICT there's no this context, so at least that part of your detection is wrong.

triggers a similar diagnostics

I personally prefer "to".

? What's the preferred way of handling aliases in diagnostics?

Why not mention both? "assignment of g_x to g_y ..."

[nordlow]

AFAICT there's no this context, so at least that part of your detection is wrong.

I thought so too at first but look again. The indentation is flattened. The scope of X is indeed a struct. Search for the struct keyword and you will find it.

[rainers]

test/compilable/interpret3.d(6245,15): Error: construction of member this.coord from itself

This looks like a bug to me (showing the merits of your PR).

The test is against an ICE with the assignment, so it should not be removed, but replaced with something similar, e.g. Coord13831 x; coord = x;.

test/runnable/extra-files/paranoia.d(1530,5): Error: assignment of member this.X from itself

AFAICT there's no this context, so at least that part of your detection is wrong.

Most of the module is wrapped in a struct, so the message is ok. The assignment can probably be removed.

[nordlow]

The test is against an ICE with the assignment, so it should not be removed, but replaced with something similar, e.g. Coord13831 x; coord = x;.

Can you give a complete replacement for

struct Chunk13831
{
    this(Coord13831)
    {
        coord = coord;
    }

    Coord13831 coord;

    static const Chunk13831* unknownChunk = new Chunk13831(Coord13831());
}

, please? @rainers?

My guess is

struct Chunk13831
{
    this(Coord13831)
    {
        Coord13831 x;
        coord = x;
    }

    Coord13831 coord;

    static const Chunk13831* unknownChunk = new Chunk13831(Coord13831());
}

. Is this ok?

[dkorpel]

@nordlow Are you still pursuing this?

[nordlow]

@nordlow Are you still pursuing this?

Not at the moment, no. I'm uncertain about the scope of the implementation.