Bug 21674 – [REG v2.086] `alias this` triggers wrong deprecation message on function call

Originally posted as a comment on the PR that introduced the regression: https://github.com/dlang/dmd/pull/9323#issuecomment-686852776 ``` struct Module { CachedString data; } struct CachedString { private size_t len; this (string data) { this.len = data.length; } public string str () const { return null; } public void str (string value) { this.len = value.length; assert(0); } alias str this; } void main () { Module m; m.data = "Hello World"; } ``` I added an `assert(0);` to confirm that `str` is being called (and it is). This triggers the following deprecation message: ``` dep.d(20): Deprecation: Cannot use `alias this` to partially initialize variable `m.data` of type `CachedString`. Use `m.data.str` ``` This message is wrong, the `str` setter gets called (as expected).

It's a feature :) *** This issue has been marked as a duplicate of issue 19441 ***

In the provided example, there is no assignment taking place, the compiler is calling `str(string)`, so it is not "a feature". I just read the initial issue again, and *if* we forbid `alias this` on the LHS, the code I showed would still be valid. Looking at the deprecation, it is expected to be turned into an error, which is why I marked it as `rejects-valid`. In Johan's last message, the code should really not need the `opAssign` in S1. P.S: As a side note, issues that one would deem "a feature, not a bug" should not be marked as duplicate of issue that change a behavior, but rather, INVALID, as their premise is wrong.

Assignment in your example is m.data = "Hello World";

Please read the bug report again, or at the very least run the code. As I mentioned in the OP: > I added an `assert(0);` to confirm that `str` is being called (and it is). Because one can't assign a `string` to a `CachedString` (the only member is length and the `alias this` is on a function).

If the compiler can compile it, it doesn't mean it should do so, because indiscriminate compiling can lead to unintended behavior. AIU issue 19441 is based on a bug in weka where aliased setter wasn't intended to customize assignment. There was grumbling about aliased getters too, but they are too useful and there's nothing to replace them with, but aliased setters duplicate functionality of opAssign in a sloppy way.

Supplementary test for this issue. --- EntryType arr; auto getPtr() { return &arr; } struct EntryType { //int somethingelse; // add for different error message bool _state; alias _state this; } struct S1 { @property auto ref entry() { return *getPtr(); } alias entry this; } void main(){ S1 s1; s1 = true; // Deprecation: Cannot use `alias this` to partially initialize // variable `s1` of type `S1`. Use `s1.entry()._state` } --- This happens because op_overload is recursive, i.e: op_overload(e=`s1 = true`, ad1=`S1`) -> op_overload(e=`s1.entry()`, ad1=`EntryType`) -> check: EntryType.members == 1 && typeof(EntryType[0]) == typeof(true) -> TRUE: return e=`s1.entry()._state = true` -> check: S1.members == 1 && typeof(S1[0]) == typeof(true) -> FALSE: deprecation warning

The cause of the regression is that the conditions that check this are looking at the wrong thing. --- if (Expression result = checkAliasThisForLhs(ad1, sc, e)) { // Problem #1 // + e = `m.data = "Hello World"` // + result = `m.data.str("Hello World") // I think we should instead be looking at the `result{.e1}.op`, not the // original expression. if (e.op != EXP.assign || e.e1.op == EXP.type) return result; // Problem #2 // + ad1 = `S1` // + result.var.parent = `EntryType` // I think we should instead be looking at the aggregate declaration for // the LHS of the rewritten `result`. if (ad1.fields.dim == 1 || (ad1.fields.dim == 2 && ad1.vthis)) { if (var && var.type == ad1.fields[0].type) return result; if (tf.isref && ad1.fields[0].type == tf.next) return result; } } ---

What would be considered the right behaviour for statics? --- struct Test { private static int _impl; static ref int state() { return _impl; } void check(int expected) { assert(_impl == expected); } alias foo this; } struct Test2 { Test test; alias test this; } Test t; t = 42; // foo() = 42 t.check(42); Test2 t2; t2 = 42; // foo() = 42; t2.check(42); --- Of course specs are not too specific about it. Based on the content of issue 19441, my intuition would be don't deprecate it, because we only care about the DotVarExp cases.

(In reply to Iain Buclaw from comment #7) > if (tf.isref && ad1.fields[0].type == tf.next) > return result; The more I look at this, the more it does not make any sense to me.

@ibuclaw created dlang/dmd pull request #13681 "fix Issue 21674 - [REG v2.086] 'alias this' triggers wrong deprecation message on function call" fixing this issue: - fix Issue 21674 - [REG v2.086] 'alias this' triggers wrong deprecation message on function call https://github.com/dlang/dmd/pull/13681

dlang/dmd pull request #13681 "fix Issue 21674 - [REG v2.086] 'alias this' triggers wrong deprecation message on function call" was merged into stable: - c9abd7395b75e955803558763464df3f4a56fd5f by Iain Buclaw: fix Issue 21674 - [REG v2.086] 'alias this' triggers wrong deprecation message on function call https://github.com/dlang/dmd/pull/13681

dlang/dmd pull request #13685 "Merge stable into master" was merged into master: - 1fdbf22f7a9bf5c4fd721a30b0099a5f33ef8323 by Iain Buclaw: fix Issue 21674 - [REG v2.086] 'alias this' triggers wrong deprecation message on function call https://github.com/dlang/dmd/pull/13685