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

Implement adjusted member conflict rules #33235

Open
1 of 3 tasks
eernstg opened this issue May 25, 2018 · 3 comments
Open
1 of 3 tasks

Implement adjusted member conflict rules #33235

eernstg opened this issue May 25, 2018 · 3 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).

Comments

@eernstg
Copy link
Member

eernstg commented May 25, 2018

(Edit May 31st by eernstg: The member conflicts rules have been simplified considerably, which is reflected in the simpler text below. E.g., many rows in the table are now justified by the same line number in the specification.)


Cf. issue #33077, CL 54410, and commits e8bb129 and 08c893d, the Dart 2 rules for which pairs of member declarations constitute a conflict have changed slightly, and expressed in a significantly simpler form. This issue is concerned with the implementation and testing of the updated rules.

For instance, it is now an error if a class declares a setter named v= and has a method named v, which will make the two declarations of m here a conflict. Here is a complete table showing the member declaration conflicts with the new ruleset:

Member1                       Member2                       Conflict Reason
-----------------------------------------------------------------------------
declares constructor `C.n`    declares static getter `n`    2449
declares constructor `C.n`    declares static method `n`    2449
declares constructor `C.n`    declares static setter `n=`   2449
declares static getter `n`    declares static method `n`    346, 2450, and 2451
declares static getter `n`    has instance getter `n`       2452
declares static getter `n`    has instance method `n`       2452
declares static getter `n`    has instance setter `n=`      2452
declares static method `n`    has instance getter `n`       2452
declares static method `n`    has instance method `n`       2452
declares static method `n`    has instance setter `n=`      2452
declares static setter `n=`   declares static method `n`    2450 and 2451
declares static setter `n=`   has instance getter `n`       2452
declares static setter `n=`   has instance method `n`       2452
declares static setter `n=`   has instance setter `n=`      2452
has instance getter `n`       has instance method `n`       2450 or 2451, possibly 346
has instance setter `n=`      has instance method `n`       2450 or 2451

The number in the third column is the line number in dartLangSpec.tex as of c6129f7 where the given situation is specified to be a conflict (that is, a compile-time error or a static warning (which is also an error in Dart 2)).

The conflicts listed above are concerned with conflicts among member declarations that are beyond the simple name clash where two declarations introduce the same name into the same scope, that is, the names may be different, and they may be in different scopes.

In particular, the above list says nothing about overriding (because overriding is always concerned with two declarations of the same kind, and the list does not mention any such case).

It may be useful to consider certain cases where there is no conflict:

Member1                       Member2
---------------------------------------------------------
declares static setter `n=`   declares static getter `n`
has instance getter `n`       has instance setter `n=`
declares constructor `C.n`    has instance method `n`
declares constructor `C.n`    has instance getter `n`
declares constructor `C.n`    has instance setter `n=`

Subtasks.

@eernstg eernstg added area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). area-multi labels May 25, 2018
@munificent munificent added area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). and removed area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). area-multi labels Jun 22, 2018
@mit-mit
Copy link
Member

mit-mit commented Jul 9, 2018

@eernstg how important is this for Dart 2 stable? Is this a breaking change or not? Do we have tests?

We are asking as this one is not in the Dart 2 milestone, but the CFE specific issue (#32613) is.

@eernstg
Copy link
Member Author

eernstg commented Jul 9, 2018

It is breaking because it makes m in #33077 (comment) an error, and also because it introduces a conflict between a named constructor and certain static members.

Almost all combinations of methods (static or instance) with the same base name (a method/getter named n has base name n, a setter named n= has base name n) were already an error, so anyone who knew that would probably not try to exploit that missing case (has method, declares setter), which means that it is expected to cause very little breakage.

I'm not aware of a concrete presubmit experiment which would reveal the breakage more precisely.

Note that #33237 (the Fasta subtask) was closed 10 days ago, which should imply that bleeding edge based usages would already have seen the breakage.

@eernstg
Copy link
Member Author

eernstg commented Jul 9, 2018

I just noticed that issue #32613 is not closed, although #33237 is closed, and also that #33237 was closed based on a commit with message 'Check for conflicts between a static setter and a constructor or static method', which does not mention the case in #33077.

Issue #32613 is not exactly the CFE subtask for this issue, it is a broader task which is concerned with the implementation of conflict checking for members (at all, they used to be completely missing), not just the changes which were introduced as of 08c893d. At this point, however, resolving issue #32613 should imply that #33237 is also resolved, because a full implementation should include the latest updates.

My current interpretation of the situation is that #33237 was closed too early (unless the remaining cases including "declares instance setter, has instance method" have been implemented in some other commit), and also that #33237 and #32613 should be consolidated (such that they do not overlap, or at least such that the overlap is known and is being managed).

@a-siva a-siva added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Jul 9, 2018
@munificent munificent removed the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Jul 11, 2018
dart-bot pushed a commit that referenced this issue Jul 16, 2018
See #32613
and #33235 (comment)

Bug: #32613, #33235, #33237
Change-Id: I0d1432185b6811137e31135ac2c7f58c4de2de6c
Reviewed-on: https://dart-review.googlesource.com/64500
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
dart-bot pushed a commit that referenced this issue Sep 4, 2018
…CONFLICTING_INSTANCE_SETTER_AND_SUPERCLASS_MEMBER.

These errors were removed from the spec.
See also #33235

R=brianwilkerson@google.com

Bug: #34331
Change-Id: Ibc675aa48165300ddac32a8d6ddef4d84d41952a
Reviewed-on: https://dart-review.googlesource.com/72903
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).
Projects
None yet
Development

No branches or pull requests

4 participants