-
Notifications
You must be signed in to change notification settings - Fork 213
Starting on partial class proposal #680
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
Conversation
Author: [email protected] Proposed solution to [partial classes (#252)](#252). Discussion about this proposal should go in [Issue #678](#678).
Motivated by https://twitter.com/QQqtNni8YTHTj9l/status/1192470981018898434 to give this a "partial" stab 😀 |
> Why not restrict it to one-definition-per-file? It's common for code | ||
generators to create one file with the output from several separate generators. | ||
|
||
### Class hierarchy and members are merged across partial classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What order are mixins applied in? This at least needs to be defined :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's merging, not mixin application. There is only one class. That also means that there cannot be any conflicts between concrete member declarations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question
class Person with A{}
class Person with B{}
class Person with C{}
Should be treated as
class Person with A, B, C{}
But if the Person
definitions are across many parts, whe application of each mixin should be deterministic. Trivial thought: in order they are defined in each file, starting w/ defining library, then each part file in the order in which the directive appears in the deefining library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's consistent, even if somewhat arbitrary. It will be the only place where the order of declarations matter, so I'd prefer to not go there, and simply not allow a partial class part to add a mixin any more than I'd allow it to change the superclass otherwise.
An ideal solution would provide: | ||
|
||
- Strict separation between human- and computer-generated code. | ||
- Allow computer-generated code to add members (functions, fields, properties, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does have some compile time implications - all codegen has to be done before a library can be summarized/outlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Adding stubs now.
final DateTime dateOfBirth; | ||
Person({this.firstName, this.lastName, this.dateOfBirth}); | ||
|
||
// No special wiring needed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at this class as a user how would I know the toJson/fromJson exist? The annotation is kind of a hint but annotations aren't required here.
You would have to click through to the part file to see these extra memebers and there isn't any indication that there are additional members.
This is less boilerplate though for the user 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea. :)
> Why not restrict it to one-definition-per-file? It's common for code | ||
generators to create one file with the output from several separate generators. | ||
|
||
### Class hierarchy and members are merged across partial classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's merging, not mixin application. There is only one class. That also means that there cannot be any conflicts between concrete member declarations.
|
||
### Open questions | ||
|
||
* Do we want a `partial` keyword or similar? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already treat part
as a built-in identifier, it might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it's a built-in identifier. I think it might be useful.
part class Foo {
int theFoo() => 42;
}
I think it reads decently.
// before: User-generated code | ||
import 'package:json_annotation/json_annotation.dart'; | ||
|
||
part 'example.g.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this part
declaration needs to be modified too, so tools won't complain if it isn't there yet. Currently, this file is just invalid Dart when example.g.dart
does not exist. We likely want to continue complaining about missing imports, so it might be necessary to state that this file can be omitted temporarily for static analysis.
It is still worrysome that your code might change when importing the part file.
If the part file declares a public name, it might start shadowing an imported name, and the meaning of code changes between analysis without the part and analysis with the part.
Same can happen for identifiers inside instance members which suddenly see part-file introduced members that weren't there before.
It also means that code using the generated API still can't be analyzed before the part is available. If the C.foo
method is only introduced by the generated code, with no stub, then any code which does C c = ...;c.foo();
will not be correct without the generated file. You can add the stub, but then you really have to add all the stubs, which does not seem practical (or the goal of this approach).
Or perhaps the analyzer could have a mode where it can see that some import is missing and the library contains partial classes, so it should assume that any non-conflicting member access on that class is going to be fine (no more than a hint that
The C.foo member is not available yet. The following files are still missing: "example.g.dart"
or similar). It won't work if the entire class is introduced in the part
.
The Dart system patch files are similar in some ways and they cannot introduce new public names, and the library itself cannot see private names from the patch file (it can define an external
private member first, then the patch can provide the implementation). This also means that the generated code can use any private name for its implementation without risking conflicts.
That approach conflicts with goal of allowing generated code to add public members which do not have stubs, though.
- The "shape" of `Person` is now visible to static analysis tools even before | ||
code generation is ran. | ||
|
||
### Open questions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about static members:
// file1.dart
part "part1.dart";
class C {}
// part1.dart
part of "file1.dart"
class C {
static int counter = 0;
}
Since you can add constructors, you should also be able to add static members.
|
||
```dart | ||
class Person { | ||
int get value => 42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the signatures do not match?
Say:
class P {
external num get foo;
}
class P {
int get foo => 42;
}
Is this valid or an error?
It could be valid, with the P
interface having either num get foo;
or int get foo;
as member signature and the implementation being int get foo => 42;
, a valid implementation of either interface.
There probably has to be a most specific signature among the available ones, just as if we were merging different interfaces, so I'd go with int get foo;
for the interface too.
That, or disallowing any difference.
Random thought I was having here - how much value does this provide over extensions? Should we improve extensions instead so you can add constructors and static methods? |
I think this feature could provide much that static extension methods do not. It's a different feature solving a different problem. Partial classes are generally brought up in the context of generated code. If you have class which includes both generated members and pre-written members, then you currently have to either rewrite the class around the pre-written members or add one of the kinds of code to the other using mixins. The code gets muddled in unrelated glue features and you often need to repeat names to do the linking. A partial class declaration would allow a class to be declared in total by code living in two different files. Maybe, even likely, files of the same library, but it still allows you to generate an entire file at a time and get automatic linking without the two parts having to explicitly mention each other. The methods will still be first-class virtual methods, not static extension methods. |
I understand the difference - but I think extension methods could still solve a lot of the same problems if they were more powerful. If you define an extension in the same library as a class then it starts to look pretty similar to actual methods on the class. Obviously you can't add fields and whatnot, but the virtual vs static dispatch distinction matters less in that case in a practical sense. That doesn't mean its not worth doing this - it is a very different feature to be sure. |
FYI: I'm excited about this feature, but I don't plan to push on it until at least Q2 (April) 2020. Hoping I can find someone a bit more clue-full about language design to take it on 😄 |
Hello, thank you for this proposal! I have one question though. // after: User-generated code
import 'package:json_annotation/json_annotation.dart';
part 'example.g.dart';
@JsonSerializable(nullable: false)
class Person partial {
final String firstName;
final String lastName;
final DateTime dateOfBirth;
Person({this.firstName, this.lastName, this.dateOfBirth});
// These stubs ensure the shape of the type is fully visible before code
// generation to enable static analysis. The stubs can also be available
// in the analyzer API allow code generation to use them. For instance:
// json_serializable could drop the explicit annotations to enable/disable
// factory and/or toJson members and instead just key off the existence of
// the corresponding stubs.
partial factory Person.fromJson(Map<String, dynamic json>);
partial Map<String, dynamic> toJson();
} the stubs are optional, right? |
@kevmoo any news? |
Not yet. We're all working really hard to get null safety finished. What scenarios are you looking to use partial classes with? |
@kevmoo for code generation. |
That's our inspiring scenario. Just double-checking. 👍 |
Ditto here @munificent – happy to close this out RE https://github.com/dart-lang/language/blob/master/working/augmentation-libraries/feature-specification.md SGTM! |
Closing as stale. We'll continue to explore augmentation libraries (linked from the previous comment) |
Author: [email protected]
Proposed solution to partial classes (#252).
Discussion about this proposal should go in Issue #678.