phobos icon indicating copy to clipboard operation
phobos copied to clipboard

fix issue 18436 - broken opCast fails silently when used with std.conv.to

Open Herringway opened this issue 6 years ago • 7 comments

I'm reasonably certain this change won't cause previously-rejected cases to be accepted or vice versa.

Herringway avatar Feb 14 '18 03:02 Herringway

Thanks for your pull request and interest in making D better, @Herringway! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
18436 enhancement broken opCast fails silently when used with std.conv.to

dlang-bot avatar Feb 14 '18 03:02 dlang-bot

Can you post what the new error message would be?

JackStouffer avatar Feb 14 '18 14:02 JackStouffer

Hmm, ideally we would add a testcase to prevent regressions, but it doesn't look like that's easy to do here. Maybe we can add a file to the dmd's testsuite? (there are quite a few tests that import Phobos already)

wilzbach avatar Feb 14 '18 17:02 wilzbach

The new error message(s) would depend on the reason why S.opCast!T failed to compile, as you would get when calling it directly.

struct X {
  T opCast(T)() {
    someFunctionThatDoesntExist();
    return T.init;
  }
}

Without the change, X().to!bool fails to match any of the toImpls and the compiler will not print any errors about the opCast. I've had to litter code with non-templated functions explicitly instantiating X.opCast!T just to be able to see why it's failing to compile. With this change, that stops being necessary and the compiler will print any errors that occur while instantiating X.opCast!T.

Other parts of phobos suffer from the same problem, but this was the easiest one to resolve that I've identified so far.

Herringway avatar Feb 14 '18 21:02 Herringway

Right, but can you post the before and after of the error messages?

JackStouffer avatar Feb 14 '18 21:02 JackStouffer

Before:

/usr/include/dlang/dmd/std/conv.d(208): Error: template std.conv.toImpl cannot deduce function from argument types !(bool)(X), candidates are:
/usr/include/dlang/dmd/std/conv.d(456):        std.conv.toImpl(T, S)(S value) if (isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T))
/usr/include/dlang/dmd/std/conv.d(570):        std.conv.toImpl(T, S)(ref S s) if (isStaticArray!S)
/usr/include/dlang/dmd/std/conv.d(586):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && is(typeof(S.init.opCast!T()) : T) && !isExactSomeString!T && !is(typeof(T(value))))
/usr/include/dlang/dmd/std/conv.d(637):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && is(T == struct) && is(typeof(T(value))))
/usr/include/dlang/dmd/std/conv.d(686):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && is(T == class) && is(typeof(new T(value))))
/usr/include/dlang/dmd/std/conv.d(759):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && (is(S == class) || is(S == interface)) && !is(typeof(value.opCast!T()) : T) && (is(T == class) || is(T == interface)) && !is(typeof(new T(value))))
/usr/include/dlang/dmd/std/conv.d(880):        std.conv.toImpl(T, S)(S value) if (!(isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T)) && !isInfinite!S && isExactSomeString!T)
/usr/include/dlang/dmd/std/conv.d(1024):        std.conv.toImpl(T, S)(ref S value) if (!(isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T)) && !isInfinite!S && isExactSomeString!T && !isCopyable!S)
/usr/include/dlang/dmd/std/conv.d(1320):        std.conv.toImpl(T, S)(S value, uint radix, LetterCase letterCase = LetterCase.upper) if (isIntegral!S && isExactSomeString!T)
/usr/include/dlang/dmd/std/conv.d(1400):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && (isNumeric!S || isSomeChar!S || isBoolean!S) && (isNumeric!T || isSomeChar!T || isBoolean!T) && !is(T == enum))
/usr/include/dlang/dmd/std/conv.d(1493):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && !isSomeString!S && isDynamicArray!S && !isExactSomeString!T && isArray!T)
/usr/include/dlang/dmd/std/conv.d(1575):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && isAssociativeArray!S && isAssociativeArray!T && !is(T == enum))
/usr/include/dlang/dmd/std/conv.d(1818):        std.conv.toImpl(T, S)(S value) if (isInputRange!S && isSomeChar!(ElementEncodingType!S) && !isExactSomeString!T && is(typeof(parse!T(value))))
/usr/include/dlang/dmd/std/conv.d(1833):        std.conv.toImpl(T, S)(S value, uint radix) if (isInputRange!S && !isInfinite!S && isSomeChar!(ElementEncodingType!S) && isIntegral!T && is(typeof(parse!T(value, radix))))
/usr/include/dlang/dmd/std/conv.d(1892):        std.conv.toImpl(T, S)(S value) if (is(T == enum) && !is(S == enum) && is(typeof(value == OriginalType!T.init)) && !isFloatingPoint!(OriginalType!T) && !isSomeString!(OriginalType!T))
test.d(10): Error: template instance std.conv.to!bool.to!(X) error instantiating

After:

test.d(4): Error: undefined identifier someFunctionThatDoesntExist
/usr/include/dlang/dmd/std/conv.d(208): Error: template std.conv.toImpl cannot deduce function from argument types !(bool)(X), candidates are:
/usr/include/dlang/dmd/std/conv.d(456):        std.conv.toImpl(T, S)(S value) if (isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T))
/usr/include/dlang/dmd/std/conv.d(570):        std.conv.toImpl(T, S)(ref S s) if (isStaticArray!S)
/usr/include/dlang/dmd/std/conv.d(586):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && allSameType!(ReturnType!(S.opCast!T), T) && is(typeof(S.init.opCast!T())) && !isExactSomeString!T && !is(typeof(T(value))))
/usr/include/dlang/dmd/std/conv.d(638):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && is(T == struct) && is(typeof(T(value))))
/usr/include/dlang/dmd/std/conv.d(687):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && is(T == class) && is(typeof(new T(value))))
/usr/include/dlang/dmd/std/conv.d(760):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && (is(S == class) || is(S == interface)) && !is(typeof(value.opCast!T()) : T) && (is(T == class) || is(T == interface)) && !is(typeof(new T(value))))
/usr/include/dlang/dmd/std/conv.d(881):        std.conv.toImpl(T, S)(S value) if (!(isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T)) && !isInfinite!S && isExactSomeString!T)
/usr/include/dlang/dmd/std/conv.d(1025):        std.conv.toImpl(T, S)(ref S value) if (!(isImplicitlyConvertible!(S, T) && !isEnumStrToStr!(S, T) && !isNullToStr!(S, T)) && !isInfinite!S && isExactSomeString!T && !isCopyable!S)
/usr/include/dlang/dmd/std/conv.d(1321):        std.conv.toImpl(T, S)(S value, uint radix, LetterCase letterCase = LetterCase.upper) if (isIntegral!S && isExactSomeString!T)
/usr/include/dlang/dmd/std/conv.d(1401):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && (isNumeric!S || isSomeChar!S || isBoolean!S) && (isNumeric!T || isSomeChar!T || isBoolean!T) && !is(T == enum))
/usr/include/dlang/dmd/std/conv.d(1494):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && !isSomeString!S && isDynamicArray!S && !isExactSomeString!T && isArray!T)
/usr/include/dlang/dmd/std/conv.d(1576):        std.conv.toImpl(T, S)(S value) if (!isImplicitlyConvertible!(S, T) && isAssociativeArray!S && isAssociativeArray!T && !is(T == enum))
/usr/include/dlang/dmd/std/conv.d(1819):        std.conv.toImpl(T, S)(S value) if (isInputRange!S && isSomeChar!(ElementEncodingType!S) && !isExactSomeString!T && is(typeof(parse!T(value))))
/usr/include/dlang/dmd/std/conv.d(1834):        std.conv.toImpl(T, S)(S value, uint radix) if (isInputRange!S && !isInfinite!S && isSomeChar!(ElementEncodingType!S) && isIntegral!T && is(typeof(parse!T(value, radix))))
/usr/include/dlang/dmd/std/conv.d(1893):        std.conv.toImpl(T, S)(S value) if (is(T == enum) && !is(S == enum) && is(typeof(value == OriginalType!T.init)) && !isFloatingPoint!(OriginalType!T) && !isSomeString!(OriginalType!T))
test.d(10): Error: template instance std.conv.to!bool.to!(X) error instantiating

I had to undo the latest change to get that error to display correctly, so I'm going to have to rethink that fix...

Herringway avatar Feb 14 '18 22:02 Herringway

Hmm... is(typeof(S.init.opCast!T())) is needed to reject cases where opCast can't be instantiated with T due to constraints, but also rejects cases where opCast can't be instantiated due to compile errors...

Unless there's already a way to test the constraints of a template without attempting to instantiate it, I think this is blocked for now.

Herringway avatar Feb 15 '18 02:02 Herringway