Fix Issue 9274 and 11499 - is + alias this = wrong code #8839
Conversation
|
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 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
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8839" |
There was a problem hiding this comment.
Looks good on first look, though the ocean failure may be a genuine one:
./src/ocean/core/Traits.d(2025): Error: static assert: "Argument has no parameters."
./src/ocean/core/Traits.d(2036): instantiated from here: `ParameterTupleOf!(MyType)`
./src/ocean/core/Traits.d(2064): instantiated from here: `ParameterTupleOf!(foo)`
submodules/makd/Makd.mak:595: recipe for target '/var/lib/buildkite-agent/builds/buildkite-agent-05-1/dlang/dmd/build/sociomantic-tsunami-ocean/build/production/tmp/allunittests' failed
| } | ||
|
|
||
| static assert(!is(S == float[])); // ok | ||
| static assert(!is(S == T[], T)); // fails |
There was a problem hiding this comment.
It's probably tested elsewhere, but I'd like to see positive tests next to the negative ones for completeness:
static assert(is(S : float[]));
static assert(is(S : T[], T));
| } | ||
|
|
||
| static assert(!is(B == A!int)); // OK | ||
| static assert(!is(B == A!X, X)); // assertion fails |
|
@ZombineDev It seems that sociomantic-tsunami/ocean is taking advantage of the bug. The test case is the following: template Typedef(T, string name)
{
mixin(`
enum Typedef =
("static struct " ~ name ~
"{ " ~
"alias IsTypedef = void;" ~
T.stringof ~ " value; " ~
"alias value this;" ~
"this(" ~ T.stringof ~ " rhs) { this.value = rhs; }" ~
" }");
`);
}
template ParameterTupleOf ( alias fn )
{
alias ParameterTupleOf!(typeof(fn)) ParameterTupleOf;
}
template ParameterTupleOf( Fn )
{
static if( is( Fn Params == function ) )
pragma(msg, "function");
else static if( is( Fn Params == delegate ) )
pragma(msg, "delegate");
else static if( is( Fn Params == Params* ) )
pragma(msg, "pointer");
else
static assert( false, "Argument has no parameters." );
}
void main()
{
mixin (Typedef!(void function(int, char[]), "MyType"));
MyType foo;
alias ParameterTupleOf!(foo) Params;
}As you can see the type of foo is not a function, a delegate or a pointer; it's a struct and since I don't know what to do here, since a deprecation doesn't seem to make any sense because we are not making code that did not compile to compile (in the general case), but we are silently changing behavior. Maybe output a warning? Or just mandate the faulting code to change the |
Can we deprecate the existing behavior? |
|
@jacob-carlborg As I stated earlier, it's a bit weird to trigger a deprecation, because we would do it on compilable code. If folks used an is expression on a struct with an alias this we are now changing the behavior of the code, we are not triggering an error. |
|
How about a transition switch? |
|
Transition switches haven't really worked in the past. Only when they were added to temporarily retain a behavior ... |
|
This is accept-invalid, we must simply fix it. |
|
(I'm assuming BuildKite was red because this PR wasn't updated after the tsunami bug was fixed. If its still red please remove the auto merge) |
You are right, but I'm not sure what the procedure with the tsunami update is (i.e. if there's a waiting time before the changes from the sociomantic repo get pushed to the project tested by buildkite) cc @mihails-strasuns-sociomantic Later edit: seems that the tsunami repo passes all tests now. This is good to merge |
|
|
buildkite agent picks latest released tag automatically so it is simply a matter of waiting until release with the fix is out and restarting the build |
|
@RazvanN7 Or not, semaphore timed out. Please merge it when DAutoTest is done, I'm going to sleep. @mihails-strasuns-sociomantic Thanks! Good to know. |
deduceType does what its name suggests : it deduces types. While doing so, it also checks for alias this, but it leaves no trace of whether the argument was matched through alias this or not. IsExp semantic uses deduceType for complex patterns therefore when it analyzes the code in the bug report it bounds the deduced type to the alias this leaving no trace of this whatsoever.
@andralex @WalterBright this is yet another example where having an alias this match would prove useful, but since we do not have one, this workaround is employed.