Fix: isZeroInit does not take into account unions

[ntrel]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @ntrel! 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
✓	23841	normal	isZeroInit does not take into account unions
✓	24776	normal	Struct with anonymous union has wrong isZeroInit

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

[kinke]

I guess the current problem (CI errors) would be uncovered by adding a regular zero-initialized field to S7 here, before the anonymous union. That shows that you cannot simply break out of the loop if the 2nd field happens to be overlapped.

[ntrel]

Yes. There doesn't seem to be a way to detect anonymous unions easily at the semantic stage. So this no longer fixes anonymous unions but does fix named unions.

[ntrel]

P.S. It was weird that the CI didn't seem to be reporting clear errors about what had broken.

[kinke]

There doesn't seem to be a way to detect anonymous unions easily at the semantic stage.

Perhaps checking whether the byte offset is the same as the previous field's - and whether the field is overlapped, due to bitfields. Edit: Oh, and that the previous field isn't empty (T[0]).

[ibuclaw]

Yes. There doesn't seem to be a way to detect anonymous unions easily at the semantic stage. So this no longer fixes anonymous unions but does fix named unions.

FYI fields is a flattened array of all found field vars, but members is the full parse tree. There's an extra cost to traverse recursively over all members though.

[ntrel]

Thanks both for the info. Perhaps I will look at fixing anonymous unions in another pull.

and that the previous field isn't empty (T[0]).

I didn't realize that, taking account of it (seems to have) fixed this pull.

[ntrel]

fixing anonymous unions

Now done!

[kinke]

Nice! - I meant the previous field's size though; I guess this fails (wrongly treated as zero-initialized):

struct {
    int[0] dummy; // same offset as anon union, but doesn't overlap; should be ignored anyway for zero-init check
    union {
        float f; // is the first member of the anon union and must be checked
        int i;
    }
}

[kinke]

[So a simple early if (vd.type.size(vd.loc) == 0) continue; at the iteration start should do the job, shortcutting the check for 0-length static arrays and not setting lastOffset, so that the first union member is always checked.]

[ntrel]

Oops, thanks. Added that test. It is a lot better, but there are (less common) cases where it wrongly doesn't zero init:

union U3
{
    int y;
    struct {
        float z, w; // z ignored but w has different offset to x
    }
}

If this is merged, I'll file that case separately.

[kinke]

Oof yeah, no idea how we'd easily fix those cases.