New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An overriding method can't specify a different default value #34437

Open
danrubel opened this Issue Sep 11, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@danrubel
Member

danrubel commented Sep 11, 2018

The failing co19_2 tests appear to be due to missing constant evaluation or error checking after constant evaluation should be performed. I don't think that this should occur in the fasta parser or in AstBuilder, but if so, feel free to assign it to me and I'll fix.

tests/co19_2/co19_2-analyzer.status:

  • Language/Classes/Abstract_Instance_Members/override_default_value_t01
  • Language/Classes/Abstract_Instance_Members/override_default_value_t02
  • Language/Classes/Abstract_Instance_Members/override_default_value_t03
  • Language/Classes/Abstract_Instance_Members/override_default_value_t04
  • Language/Classes/Abstract_Instance_Members/override_default_value_t05
  • /Language/Classes/Instance_Methods/override_different_default_values_t01
  • /Language/Classes/Instance_Methods/override_different_default_values_t02
@bwilkerson

This comment has been minimized.

Show comment
Hide comment
@bwilkerson

bwilkerson Sep 11, 2018

Member

@leafpetersen Are the tests above valid? (I'm guessing the answer is "yes", but would appreciate the confirmation anyway.)

Member

bwilkerson commented Sep 11, 2018

@leafpetersen Are the tests above valid? (I'm guessing the answer is "yes", but would appreciate the confirmation anyway.)

@danrubel

This comment has been minimized.

Show comment
Hide comment
@danrubel

danrubel Sep 11, 2018

Member

Apparently, this error checking was part of ErrorVerifier and removed in Remove spec mode support from ErrorVerifier. Thanks to @bwilkerson for tracking this down.

Member

danrubel commented Sep 11, 2018

Apparently, this error checking was part of ErrorVerifier and removed in Remove spec mode support from ErrorVerifier. Thanks to @bwilkerson for tracking this down.

@leafpetersen

This comment has been minimized.

Show comment
Hide comment
@leafpetersen

leafpetersen Sep 11, 2018

Member

Yes, they look right to me.

cc @lrhn

Member

leafpetersen commented Sep 11, 2018

Yes, they look right to me.

cc @lrhn

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Sep 12, 2018

Member

I agree that the test should give compile-time errors with the current specification.

It is a compile-time error if an instance method $m_1$ overrides an instance member $m_2$, the signature of $m_2$ explicitly specifies a default value for a formal parameter $p$, and the signature of $m_1$ implies a different default value for $p$.

Member

lrhn commented Sep 12, 2018

I agree that the test should give compile-time errors with the current specification.

It is a compile-time error if an instance method $m_1$ overrides an instance member $m_2$, the signature of $m_2$ explicitly specifies a default value for a formal parameter $p$, and the signature of $m_1$ implies a different default value for $p$.

@bwilkerson

This comment has been minimized.

Show comment
Hide comment
@bwilkerson

bwilkerson Sep 12, 2018

Member

Thanks!

Just to clarify, the error reporting wasn't removed in that CL. The code that referenced the error code was removed, but the removed wasn't being run when strong mode was enabled. The removed code indicates that this error was being generated elsewhere, but I have not been able to find where that was suppose to be happening (it wasn't using this error code).

What I can tell you is that these tests were marked as failing when the co19_2 tests were created on Feb 1, 2018 (https://dart-review.googlesource.com/37743). I suspect that this error was accidentally disabled at some point during the development of strong mode. In any case, it's been broken for at least 7 months.

Member

bwilkerson commented Sep 12, 2018

Thanks!

Just to clarify, the error reporting wasn't removed in that CL. The code that referenced the error code was removed, but the removed wasn't being run when strong mode was enabled. The removed code indicates that this error was being generated elsewhere, but I have not been able to find where that was suppose to be happening (it wasn't using this error code).

What I can tell you is that these tests were marked as failing when the co19_2 tests were created on Feb 1, 2018 (https://dart-review.googlesource.com/37743). I suspect that this error was accidentally disabled at some point during the development of strong mode. In any case, it's been broken for at least 7 months.

@natebosch

This comment has been minimized.

Show comment
Hide comment
@natebosch

natebosch Sep 13, 2018

Member

CFE also does not report these as errors, it will be breaking to change this to an error.

Member

natebosch commented Sep 13, 2018

CFE also does not report these as errors, it will be breaking to change this to an error.

@lrhn

This comment has been minimized.

Show comment
Hide comment
@lrhn

lrhn Sep 14, 2018

Member

If no compiler implements the restriction, and it'll be a compile-time error to re-introduce it, we may want to consider just dropping the restriction.

It's not required for soundness, and it means that a subclass can't just drop the default value and use param ??= defaultValue; in code instead, even if that actually handles explicit null arguments better.
Or a sub-class can't use a more specialized default value (say const ColorPoint(0, 0, Color.black) instead of const Point(0, 0), which makes sense for covariant parameters).

Member

lrhn commented Sep 14, 2018

If no compiler implements the restriction, and it'll be a compile-time error to re-introduce it, we may want to consider just dropping the restriction.

It's not required for soundness, and it means that a subclass can't just drop the default value and use param ??= defaultValue; in code instead, even if that actually handles explicit null arguments better.
Or a sub-class can't use a more specialized default value (say const ColorPoint(0, 0, Color.black) instead of const Point(0, 0), which makes sense for covariant parameters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment