-
Notifications
You must be signed in to change notification settings - Fork 207
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
Allow inferring the name on object patterns #2563
Comments
This can apply to both declarations and case-matches. Case matches are the easier: You know the static type of the expression being switched on ( At that point, if the static type of the incoming value cannot be a record, then an "unqualified extractor pattern" is interpreted as an extractor pattern on that static. The trickiest one is probably The assignment is more problematic, Consider: class Box<T> {
T value;
Box(this.value);
}
void main() {
var (:double value) = Box(1);
print(value);
} Here the type schema of the pattern is There is no good way to pass the expectation that I think it can work. I'm slightly irked by the "arbitrariness". For records and non-record types, it's fine. It's the supertypes of records where the choice feels arbitrary. Also, this all assumes the pattern can be either a record pattern or an extractor pattern. var (int x, int y) = new Point(1, 2); // Author forgot the `:`s Will the LHS just be a record pattern, which doesn't match? |
Yes, the original proposal worked that way. When I talked to folks, some people felt it was strange that the context type might end up being discarded and not actually the same as the eventual incoming type, but as far as I know, it wasn't actually problematic.
Yeah, but I think those cases will be rare and when they occur, the record pattern is what you want.
I was thinking that I would specify this by making the name in an extractor pattern optional. So instead of saying a record pattern might behave like an extractor, we'd specify that an extractor might look like a record. Then we'd just disambiguate the colliding syntaxes by looking at the matched value type. But on further thought, that's probably the wrong way to go about it. Because that means we can't disambiguate the syntax until we've done type analysis. So... I'm not sure. I'll have to think about how to specify this cleanly once my brain is more COVID-free and functional. But, yes, the above case would end up being an error. |
After talking a bit with @mit-mit about #1201 , it occurred to me that allowing this could also make field promotion using class LongNamedThing {
int? longNamedField;
}
void myCode(LongNamedThing thing) {
if (thing.longNamedThing != null) {
print(thing.longNamedThing!.isEven);
}
} With the patterns feature, you can rewrite this as follows: void myCode(LongNamedThing thing) {
if (thing.longNamedThing case var longNamedThing?) {
print(longNamedThing.isEven);
}
} which is marginally better, but still requires saying "longNamedThing" twice. Another option would be to write: void myCode(LongNamedThing thing) {
if (thing case LongNamedThing(:var longNamedThing?)) {
print(longNamedThing.isEven);
}
} but now you have to write With the proposal in this issue, you could presumably write: void myCode(LongNamedThing thing) {
if (thing case (:var longNamedThing?)) {
print(longNamedThing.isEven);
}
} which is at least marginal better. |
I think this would go a long way toward making patterns smoother to use. With regards to conflicts with records, what about changing class Foo {
Bar? get bar;
}
class Bar {
String name;
}
...
switch (Foo()) {
case #(bar: #(name: "John"): print('Hello John');
} |
That's an interesting idea. We did spend a bunch of time discussing this syntax and various other ways to make record patterns (and expressions) syntactically distinct. The other place you run into is single-element positional records: var number = (1);
var recordOfNumber = (1,); Here, the trailing comma is needed to avoid ambiguity with a parenthesized expression. A different record syntax would avoid that overlap. But in almost all other widely used languages with tuples and tuple patterns, the syntax is either a parenthesized comma-separated list of fields, or just a comma-separated list of fields. It felt like a large unfamiliarity tax to introduce a different record syntax for Dart given that. So I think the record syntax works OK. But as you suggest, we could maybe use a different syntax for "inferred object patterns". It's really hard to come up with a syntax that doesn't feel pointlessly unfamiliar, though. |
I just ran into this today and it was a bummer because the thing I'm destructuring has a multi-word name.
I totally agree. Personally, I don't hate the For another suggestion that might be more intuitive, which is what I think you're looking for, what about borrowing the switch (Foo()) {
case _(bar: _(name: "John"): print('Hello John');
} It may create some grammar ambiguities, or maybe just require some extra read-aheads to parse. That's definitely not my area of expertise, but the Given that I've just been trying to figure out all the flavors of patterns with only cursory glances at the docs, I can say pretty confidently my thought process would likely be:
|
It's a bit late, but I kind of wish Records were the ones who had an extra leading character. Like In any case, I would assume a leading character for inferred type matching should be fine. It kind of reminds me of the list.map({ it.name }); |
We discussed this, at length. But I think it would be just too weird and unfamiliar given that tuples have extremely well established syntax in so many other languages. We also discussed using
Honestly, I think the proposal at the top of this issue is the best, cleanest approach. Simply overload the nice clean record syntax to mean an object pattern when it's obvious from the static type that it can't be a record. All of the existing record patterns keep working as expected, and you get to just simply completely delete the class name in almost all of your object patterns. No additional punctuation needed. |
My opinion stems from the fact that I don't find records that useful. Hence, I don't think records as they are, justify the syntax complexity. In a way, it's as if for this feature, Dart picked the equivalent of having optional
I wouldn't do this personally. I've had to do similar var buttonColor = switch (this /* or a local variable if any */) {
MyClass(isEnabled: false, isHover: _) => lightGray, // Disabled buttons are always grayed out.
MyClass(isEnabled: true, isHover: false) => blue,
MyClass(isEnabled: true, isHover: true) => darkBlue
}; Hence this issue.
Wouldn't it possibly conflict in cases where the type is Say we do a refactoring and downcast a variable from class Class {String name};
Class obj = Class();
switch (obj) {
case (name: 'foo'): print('Hello');
case _: print('default');
} But then we do the refactor we have: class Class {String name};
-Class obj = Class();
+Object obj = Class();
switch (obj) {
case (name: 'foo'): print('Hello');
case _: print('default');
} In that case, instead of a compilation error in the |
I could be wrong, but I suspect that users will gradually find themselves using them more over time as they become more comfortable thinking in terms of them. We consider it totally natural for a function to accept multiple parameters, but most of us have had decades of habit from languages without multiple returns to expect to have to bundle the returned values up in some meaningful structure or yield the values some other way. As those habits get unlearned, I think we'll see records used more. Using records to flatten out nested pattern matching is idiomatic in functional languages, though it seems odd when you aren't used to it.
That's what the "when it's obvious from the static type that it can't be a record" part means.
Good point. |
I'm skeptical about this. After all, I'm fairly familiar with Records & co in other languages. In any case, what about In a way, this feature reminds me a bit of #357. enum MyType {a, b};
void function({MyType? parameter}) {}
function(
// sugar for "parameter: MyType.a
parameter: .a,
) Then we might as well dissociate this from enums and support the shorthand for any type such that we could write: class Example {
Example();
Example.named();
static final Example empty = Example();
}
void function({Example ? parameter}) {}
function(
// sugar for "parameter: Example.empty
parameter: .empty,
)
function(
// sugar for "parameter: Example.named()
parameter: .named(),
)
function(
// sugar for "parameter: Example()
parameter: .(),
) And I figured Example a;
switch (a) {
case .(value: 42): print('Hello world');
} In that case, the leading character would become less of an "arbitrary leading character for parsing" and more a convention for "inferred type name" |
I like this idea a lot. I've found myself writing a fair number of object-destructuring macros, and I'm a little concerned that they set me up for a situation where the type of the value being matched changes and instead of a static error the condition just silently fails to match. This would solve that issue, as well as being terser. |
That's a good point. Currently, there's no way to write a pattern that means "destructure at the current type" without doing some type test as well. Inferred object patterns would give you that. |
I think What about leaning onto existing syntax: we have already redefined Example a;
switch (a) {
case _(value: 42): print('Hello world');
} |
Maybe. It may make sense to use something other than So if it starts with _, it's a pattern. If it starts with a dot, it's a static shorthand. |
That's actually a problem. If The If It's just not a context type (scheme) here, it's more like the static type of an expression that member invocations are checked against. So it's more like I still think it can work. |
This would be especially handy for working with protos. Those classes tend to have verbose names (and submessages get very long generated names and require aliases to be at all useful), and they are often repetitive with field names. I think I'd find it easier to figure out the intent of the code without all the noise of the type names if (someProtoTypeName
case .(anotherLongName: .(protoLongName: .(:var interestingField)))
when interestingField.isNotEmpty) {
doSomethingWith(interestingField);
}
if (someProtoTypeName
case SomeProtoTypeName(
anotherLongName: AnotherLongName(
protoLongName: ProtoLongName(:var interestingField)
)
) when interestingField.isNotEmpty) {
doSomethingWith(interestingField);
} |
It would be convenient to also have a syntax that works when the static type is nullable - otherwise the type name suddenly comes back for nullable fields. Maybe a Edit: Or maybe this isn't necessary and we can use the same syntax even if the static type is nullable - checking for an object with an inferred type can be assumed to check for non-null. |
I agree. A way to have |
I think the option of using var map = {1: 'a', 2: 'b'};
for (var <>(:key, :value) in map.entries) {
...
} Another variant discussed there is a dot: var map = {1: 'a', 2: 'b'};
for (var .(:key, :value) in map.entries) {
...
} One of the objections is that the dot is not visible enough to immediately convey the notion of a missing (inferred) type. The convention has far-reaching consequences for other contexts - e.g. for default constructor calls, as discussed in #357, |
My main argument for the That being said, the syntax is more of an experiment to see which syntax could possibly work, not really something i push for. Edit: i explained it badly, the |
The term "inferred" may have a broad meaning. We have to distinguish between "inferred from its content" ("from the inside") and "inferred from the wider context" ("from the outside"), so This convention would allow the shortcut otherwise known as We must treat the whole thing (Maybe |
IMO |
@mateusfccp : treat |
Any chance we could just have: var map = {1: 'a', 2: 'b'};
for ([key, value] in map.entries) {
...
} |
@jtmcdole This issue would allow you to write With that change, you could write |
The nice thing about for (var (key: name, value: age) in people.entries) ... With for (var (name, age) in people.pairs) ... I still think we should do this feature request too, but for maps, I think |
An issue with It's a little annoying that you can't use (Eventually we can change |
Does the |
Maybe. The only way to write it efficiently as an extension method is to use the If it doesn't use |
An earlier version of the patterns proposal used record pattern syntax like
(foo: var f, bar: 3)
to match any object, and it would simply call the corresponding getters on whatever static type the matched value had. Alas, this left users with no way to actually test if an object is a record (#2218). We fixed that by saying that record pattern only ever matches record objects. If you want to call getters on other objects, you have to use a named object pattern.This works, but it's verbose in cases where you want to use an object but the type you're destructuring is the same as the matched value type, like:
Here, we already know the static type of the matched value is
MapEntry
. And we know that the value can never be an actual record sinceMapEntry
isn't a record type. So the object name doesn't add much value here. It would be nice if you could write:Of course, that's ambiguous with record patterns. To disambiguate, I would say:
You can omit the type name in an object pattern only when the matched value type is not a record type,
Record
, or a supertype ofRecord
. When the matched value is any of those types, then the pattern is instead treated as a record pattern that only matches records.When the matched value is any other type, a record pattern could never successfully match since the value will never be a record, so we instead treat it as an extractor pattern whose inferred type is the matched value type.
We do have to decide this now because the proposal currently doesn't make it an error to have a record pattern in a context where the matched value type means no record will ever appear. You can still write the record pattern, but it just will never match. When we full specify exhaustiveness checking, we might be able to flag this as an unreachable case. If we do that, then we can allow inferred object names in a later release without it being a breaking change.
The text was updated successfully, but these errors were encountered: