Skip to content
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

Fasta allows unsound abstract override #32014

Closed
ErikCorryGoogle opened this issue Jan 31, 2018 · 4 comments
Closed

Fasta allows unsound abstract override #32014

ErikCorryGoogle opened this issue Jan 31, 2018 · 4 comments

Comments

@ErikCorryGoogle
Copy link
Contributor

@ErikCorryGoogle ErikCorryGoogle commented Jan 31, 2018

This is the same missing error message that DDC and the analyzer has. See #30568 for details.

@askeksa-google
Copy link
Contributor

@askeksa-google askeksa-google commented Apr 13, 2018

The check for missing implementations in https://dart-review.googlesource.com/c/sdk/+/43660 fails to detect this case, where a concrete method is overridden by (via a subclass or implemented interface) an abstract method with a more general signature (i.e. adding optional parameters).

@askeksa-google
Copy link
Contributor

@askeksa-google askeksa-google commented Apr 13, 2018

Note that in this case, noSuchMethod forwarders are not generated, so this is an error whether or not the class has a noSuchMethod implementation different from the one in Object.

@dart-bot dart-bot closed this in 03c7184 Aug 30, 2018
dart-bot pushed a commit that referenced this issue Aug 30, 2018
…ixin."

This reverts commit 03c7184.

Reason for revert: Flutter doesn't build with after this CL because the following (kind of) program

  abstract class RenderObject {
  String toString({int minLevel}) => "foo";
  }
  abstract class ContainerRenderObjectMixin extends RenderObject {}
  abstract class RenderBoxContainerDefaultsMixin implements 
  ContainerRenderObjectMixin {}

is wrongfully rejected. In patching this, I stumbled upon what seems to be bug in Fasta where some concrete classes arising from mixed in applications are marked as abstract, which causes the patch to produce false-positives. This needs some more investigation.

Original change's description:
> Enables arity check for overridden methods inherited from a mixin.
> 
> The return type and parameter types checks are still disabled for
> mixins as there is an issue with type parameters being uninstantiated
> in mixin applications which causes the two checks to produce
> false-positives. As a result the SDK fails to compile with the two
> checks enabled because the libraries (such as collections) makes
> extensive use of mixin application with generic types. I've opened a
> separate ticket to track this issue [c.f. #34285].
> 
> Furthermore this CL abstracts the arity check, return type check, and
> method parameter type check in checkMethodOverride to make it easier
> to understand the logic of the method.
> 
> Closes #34235 and closes #32014
> 
> Change-Id: Iae224926c2e99e6e89ccc3c19ec4bc7919ee48a5
> Reviewed-on: https://dart-review.googlesource.com/71781
> Commit-Queue: Daniel Hillerström <hillerstrom@google.com>
> Reviewed-by: Aske Simon Christensen <askesc@google.com>

TBR=askesc@google.com,hillerstrom@google.com

Change-Id: Idee6f53c2d6a17ea8a4103872b1183c01e4d30ce
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/72108
Reviewed-by: Daniel Hillerström <hillerstrom@google.com>
Commit-Queue: Daniel Hillerström <hillerstrom@google.com>
@askeksa-google
Copy link
Contributor

@askeksa-google askeksa-google commented Sep 6, 2018

Partially fixed in 40864cc (only checks for difference in arity; misses checks for named parameters and types). The partial fix reuses the error message for missing members, which is misleading (especially the tip). The fix for the remaining checks should add one or more error messages to use for all mismatch situations.

@askeksa-google
Copy link
Contributor

@askeksa-google askeksa-google commented Sep 18, 2018

Fixed in https://dart-review.googlesource.com/c/sdk/+/74642, but blocked on dart-lang/dartdoc#1755 being rolled into the SDK deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dart Front End
In Progress
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants