Issue 3438 - default ctor with all default arguments must be rejected

[ghost]

http://d.puremagic.com/issues/show_bug.cgi?id=3438

[ghost]

The diff view is unhelpful unfortunately.

[yebblies]

The diff view is unhelpful unfortunately.

https://github.com/D-Programming-Language/dmd/pull/1397/files?w=0

[9rnsr]

This is not good change.

• For classes, just only this() is the default constructor. Because ad->defaultCtor is used for TypeInfo (see TypeInfo_Class.defaultConstructor).
• For structs,
  • typesafe variadics like this(int[] arr...) should also be rejected.
  • allowing @disable this(int x = 0); is seems unnatural to me.

[ghost]

Ok let me just see if I got all the test-cases before I commit. These are failures for structs:

struct F1 { this(int x = 1) { } }
struct F2 { this(int x = 1, ...) { } }
struct F3 { this(...) { } }
struct F4 { this(int[] x...) { } }
struct F5 { @disable this(int x = 1); }

And the new @disable message will be: Error: constructor test.K2.this default constructor for structs only allowed with @disable, no body, and no parameters

I'll fix classes as you mentioned. Is this ok so far?

[9rnsr]

Seems OK.

[ghost]

Damn I have no idea what happened to this, I guess we just forgot about it 9 months ago. I'll rebase it now.

[ghost]

Updated. Requesting review. @9rnsr

[ghost]

This is strange, why would merging fail? s/would/does

[9rnsr]

I finally think rejecting the definition of zero-arg callable constructors is not good, because it would reject correct use cases.

Instead, how about to reject the call of the constructors with zero-arguments. For example:

struct X { this(int x = 0) {} }  // If only X(num) is used, this is still valid definition.
// Reporting warning for the definition would be useful

struct S { this(int[] values...) {} }   // I think this is still legitimate signature
// In this case, reporting warning for the definition is not good.
void main() {
    S s1;  // OK, default constructed
    S s2 = S(10);  // OK, constructor is called (I think this is legitimate use case)
    S s3 = S();  // Changed to NG instead of the silent default construction
}

[yebblies]

I finally think rejecting the definition of zero-arg callable constructors is not good, because it would reject correct use cases.

I don't see the point of that. Why allow defining a function with all-default args, when it can never be called like that?

[9rnsr]

@yebblies I think that immediately disallow struct X { this(int x = 0) {} } will break existing valid code. We should go proper deprecation steps to disallow such definitions.

[yebblies]

Sure, making it a warning first would be a good idea. But the whole point is that code isn't valid because the default argument is dead code.

[9rnsr]

And, this(int[] values...) is still valid and must not be disallowed. It is legitimate signature, and its invalid use should be checked at its called point.

[ghost]

And, this(int[] values...) is still valid and must not be disallowed.

But this is still problematic as the user would expect the ctor would be called without arguments. See:

void test(int[] values...) { }

void main()
{
    test();  // works fine
}

Compared to:

struct S
{
    this(int[] values...) { assert(0); }  // doesn't get called
}

void main()
{
    auto s = S();
}

@yebblies I think that immediately disallow struct X { this(int x = 0) {} } will break existing valid code.

@9rnsr: You already broke a ton of code with your static opCall change..

[9rnsr]

Hmm. The essential issue I think is that, if S() exists, compiler cannot judge that it means whether the built-in default construction, or the call of zero-arg callable user-defined constructor. If a user expects the former, it should be allowed. Based on the thought:

struct S {
   this(int x = 0, int y = 0) {}   // A
   this(int[] values...) {}  // B
}

The definition of A can be immediately disallowed - removing the first default argument won't break existing code meaning, but disallowing B would be overkill, because it would need more rewriting of the body of B.

[ghost]

I guess variadic-style arguments in a ctor are used rarely enough that we could implement a special-case for them. In the vast majority of cases when users complain about "ctor not being called" they provide the example A.

[ghost]

Hmm I'm getting this in the test-suite for x64 platforms:

Error: '__va_argsave_t' is not defined, perhaps you need to import core.vararg; ?

But we're not allowed to import into druntime in the DMD test-suite. What should I use as a workaround?

[yebblies]

But we're not allowed to import into druntime in the DMD test-suite. What should I use as a workaround?

That's not true, importing druntime is fine. Importing phobos is the problematic one.

[ghost]

That's not true, importing druntime is fine. Importing phobos is the problematic one.

Ah, ok. Updated then.

[ghost]

Pinging for approval.

[ghost]

Are we planning on going forward with this? I'll rebase and reopen if it's a go.