Add a lowering field for AssignExp to store potential lowerings

[RazvanN7]

This PR does the following:

• add a field, lowering to AssignExp to store the potential lowerings that are done in the frontend.
• use lowering to lower array length expressions.
• add a new AST node LoweredAssignExp that inherits from AssignExp and adds a lowering field.
• use the new AST node to handle lowerings to array length expressions.
• delete the code in dinterpret that recreates the initial assign expression.
• update inline.d to inline the call to the lowering, not the AssignExp since that avoids extra copies.
• update hdrgen.d so that if the switch -vcg-ast is used we will print the call to the hook not the initial AssignExp. For code that does not use -vcg-ast the AssignExp is printed.
• update C++ headers
• fix Issue 23773

cc @WalterBright as this is in line with your work of not generating the hook if in ctfe blocks
cc @teodutu - this needs to be done for all previously implemented templated hooks.

[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
✓	23773	enhancement	array length assignment in assert condition should error

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

[ghost]

Is that the only solution to fix 23773 ?
I'm concerned by the memory overhead caused by this approach.

edit:

In particular and assuming there is a scope flag giving the info that you are in an assert, why not adding a check when the lowering is created ?

[RazvanN7]

No, of course there are other potential hacks that could be employed. However, this is the most general solution.
Note that the plan is to do this for all the lowered hooks.

The initial strategy was to lower the hook in the frontend (irrespective if it was in a ctfe block) and if interpretation was required the original expression would be reconstructed. Now we lower the hook only if we know that the code will be codegen-ed and we store it in an additional field.

Indeed, this comes with an extra cost, but its peanuts compared to the memory used by template instances.

[RazvanN7]

Fixing 23773 is just a collateral gain of implementing this.

[ghost]

its peanuts compared to the memory used by template instances.

expressions, including one of their most common form, Assignment ones, are also copied with template instances, i.e function bodies.

[RazvanN7]

Well, we have to agree on some path going forward. It seems that neither is ideal, so I guess it just comes down to what we want to sacrifice: compile time to create/regreate expressions for CTFE or memory to cache the lowering. In the past @ibuclaw has complained about the former approach because of the extra work at compile time and the fact that losing the original expression might affect optimizations for other backends. That is how we ended up with this strategy.

@WalterBright what is your input on this?

[ibuclaw]

The unlowering is memory expensive too.

So that not every AssignExp gets a redundant lowering field, you could make a new derived node ArrayLengthSetExp : AssignExp.

[maxhaton]

A general solution to link to where an expression was lowered from is also needed for error messages (I have worked on this in the past).

[RazvanN7]

So that not every AssignExp gets a redundant lowering field, you could make a new derived node ArrayLengthSetExp : AssignExp.

Yes, that might work, however, that will pollute the codebase with all kinds of different AST nodes, provided we will use this strategy for other lowerings as well. Anyway, compared to the importC ugliness that was added in the semantic phases I guess it's worth a shot.

[RazvanN7]

A general solution to link to where an expression was lowered from is also needed for error messages (I have worked on this in the past).

This should be solved by the approach in this PR.

[ghost]

Well, we have to agree on some path going forward

Yes, exactly. Do we waste the effect of the accumulation of the micro efforts made 2 or 3 years ago or not ? People have worked to save 10 megs here, 20 megs theres, etc. and now let's waste 80 megs because of a bug that basically nobody gives a crack about ?

[ghost]

that will affect the size of all BinaryAssign nodes btw, which are not disallowed as condition.

void main(string[] args)
{
    int a;
    if (a+= 1){} // ok
    if (a= 1){}  // ng
}  

To be clear, that PR is going nowhere, trying to fix nothing.

[RazvanN7]

@SixthDot As I've explained earlier: the fix for 23773 is just a side effect of adding the lowering field. We plan on doing variations of this approach for all of the hooks.

[ibuclaw]

@SixthDot As I've explained earlier: the fix for 23773 is just a side effect of adding the lowering field. We plan on doing variations of this approach for all of the hooks.

And hopefully a few more non-hook lowerings too. For example, I've just seen someone post this error message.

a.d(9): Error: cannot implicitly convert expression `foo(((int[7] __arrayliteral_on_stack2 = [0, 1, 2, 3, 4, 5, 6];) , cast(int[])__arrayliteral_on_stack2))` of type `int[]` to `immutable(int[])`

[RazvanN7]

@ibuclaw I have implemented the new AST node. cc @SixthDot

[ibuclaw]

I think ArraySetLengthExp would be better so that it isn't some black box the backends can't optimize for.

[RazvanN7]

@ibuclaw There are multiple lowerings that can be performed from an AssignExp. I was hoping that we could use this class for all and not add a new class for each.

[ghost]

I'm not blocking anything BTW, initially I wanted to encourage to think more about alternative solutions.

[RazvanN7]

I think ArraySetLengthExp would be better so that it isn't some black box the backends can't optimize for.

The backends can still check the original rhs and lhs expressions for optimizations. However, if you consider that the AST node should be specialized, I can do that but that will result in having an AST node for each AssignExp lowering.

[RazvanN7]

I have added the "72h no objection -> merge" label.