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

Specify private override errors #2283

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

eernstg
Copy link
Member

@eernstg eernstg commented Jun 7, 2022

Cf. #2263 (comment), this PR eliminates the notion of a 'combined interface' from the language specification and clarifies the specification of how to check override errors. The main point in doing that is that it specifies how incorrect overrides are detected and reported in the case where a class or a similar declaration in a library L gives rise to an error involving private members that aren't accessible to L.

The discussion about this change gave rise to the following summary of the rules.

I've added some remarks inline to indicate that we do in fact have these properties.

More concretely, this means that we end up with the following as the
proposed semantics for the errors and warnings around a given method
m with respect to a class declaration C in library L:

Case: m is declared in C (C is abstract, or concrete, m must be accessible in L by definition)

  • We don’t require that m have a combined signature in the super-interfaces
  • We do require that m is a proper override of all signatures for m from all direct super-interfaces

We already specify this requirement for methods, abstract as well as concrete in line 2724.

This PR moves that rule from the section 'Instance Methods' to the section 'Classes', because it is relevant to getters and setters as well (the rule was worded correctly, but it was basically misplaced).

  • The declared signature of m from C becomes the signature of m in the interface of C

This is already specified in line 4917.

Case: m is not declared in C (m may be accessible in L or not)

  • We require that a combined member signature for m WRT to the direct super-interfaces of C exist, including private members (i.e. even if m is not accessible in L, we still require that a combined member signature exist for m).

This was not stated unambiguously. This PR changes \ref{interfaceInheritanceAndOverriding} such that it is explicitly defined to be a 'member signature conflict' if an interface brings together two member signatures such that no combined member signature can be computed, and the section about classes says that it is a compile-time error if the interface of the class has any of these member signature conflicts.

This covers mixins as well, because of the phrase 'It is a compile-time error for the mixin declaration if the $M_S$ class declaration would cause a compile-time error' in the section about mixin declarations.

  • The combined member signature of m becomes the signature of m in the interface of C.

This is defined as the combined member signature of a name with respect to a library, and it is defined as mentioned here in \ref{interfaceInheritanceAndOverriding}.

  • If C is abstract, we are done: we do not require that C implement its interface
  • If C is concrete and m is public, we separately require that the concrete implementation of m implement its signature in the interface as defined above.

This is already covered by the section 'Fully Implementing an Interface'.

  • If C is concrete and m is private to a different library, we introduce a noSuchMethod thrower (forced by privacy) with the combined member signature for m as defined above.

This is already covered here.

These restrictions enforce the following properties:

Every class (abstract or concrete) has a single well-defined signature for every member (accessible or not) in its interface.
Every concrete class has a single well-defined concrete implementation of every member (accessible or not) in its interface, and that implementation correctly implements the signature from the previous bullet.

@github-actions
Copy link

github-actions bot commented Jun 7, 2022

Visit the preview URL for this PR (updated for commit 2d1bbeb):

https://dart-specification--pr2283-specify-private-over-30v5vvvy.web.app

(expires Mon, 18 Jul 2022 17:59:29 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6941ecd630c4f067ff3d02708a45ae0f0a42b88a

the checker should warn about all unimplemented methods,
but allow clients to instantiate it freely.%
the checker should raise an error for all unimplemented methods,
but clients should be allowed to instantiate it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this saying that there won't be an error reported at the instantiation point, only at the class declaration point?
If so, we don't usually say where errors are reported, we just say that "it is an error if ...." (here: "A non-
abstract class does not implement its own interface", or "A non-abstract class does not have a concrete member implementation for each member of its interface").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this says that when we have abstract class A{} any occurrences of A() will be flagged as an error, but it is not an error for A to have members in its interface that aren't implemented (with a correctly overriding signature), and the latter is an error for class A {}, but A() is allowed.

This would be the fundamental reason why we don't just say that being abstract is a computed property: If the class implements all methods then it's concrete, otherwise it is abstract. I believe that reasoning is still valid.

I think the location in the code where the error is reported is implied: When the error arises because of an instance creation A() then it will flag the location where that instance creation occurs.

I adjusted the wording a bit, I think it's more unambiguous now.

specification/dartLangSpec.tex Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Show resolved Hide resolved
specification/dartLangSpec.tex Show resolved Hide resolved
Copy link
Member Author

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review response.

Main remaining issue: Do we or do we not include commentary about topics where a compile-time error is defined in one location and mentioned in 9 other locations (because it's similar to some other compile-time error which is defined at each of those 9 other locations)?

the checker should warn about all unimplemented methods,
but allow clients to instantiate it freely.%
the checker should raise an error for all unimplemented methods,
but clients should be allowed to instantiate it.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this says that when we have abstract class A{} any occurrences of A() will be flagged as an error, but it is not an error for A to have members in its interface that aren't implemented (with a correctly overriding signature), and the latter is an error for class A {}, but A() is allowed.

This would be the fundamental reason why we don't just say that being abstract is a computed property: If the class implements all methods then it's concrete, otherwise it is abstract. I believe that reasoning is still valid.

I think the location in the code where the error is reported is implied: When the error arises because of an instance creation A() then it will flag the location where that instance creation occurs.

I adjusted the wording a bit, I think it's more unambiguous now.

specification/dartLangSpec.tex Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Outdated Show resolved Hide resolved
specification/dartLangSpec.tex Show resolved Hide resolved
specification/dartLangSpec.tex Show resolved Hide resolved
@eernstg
Copy link
Member Author

eernstg commented Jun 14, 2022

One more thing needs to be handled: We currently rely on mixins to give rise to an implicitly induced class. This class must contain a set of members which is based on the instance member declarations in the given mixin M (or, if it's a class M which is applied as a mixin: the class M).

This is only possible when we rely on the magic ability of a language processing tool to create declarations whose name is a private name in another library. So we'd need to explain how this "magic" feature can exist, except that it is only available to the compiler (in general: some feature is not available to developers who write software, only to "the system" in some sense). We can't explain the semantics of mixin application without mentioning this feature.

@lrhn
Copy link
Member

lrhn commented Jun 14, 2022

That comes back to our definition of mixin application - are we copying, or allowing the same member to occur in more than one library. (And then there's the covariant modifications happening when moving a non-covariant method implementation to a place where it implements a covariant interface method. I'm tending toward saying that the same class contains both the member declared by the mixin declaration and an override of that which adds covariant and forwards to the other method. That's two concrete methods introduced by the same class.)

@eernstg
Copy link
Member Author

eernstg commented Jul 11, 2022

July 11th: Added some extra \commentary{..} where the text seemed to need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants