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

Where is void allowed in pattern matching? #2907

Open
lrhn opened this issue Mar 13, 2023 · 9 comments
Open

Where is void allowed in pattern matching? #2907

lrhn opened this issue Mar 13, 2023 · 9 comments
Labels
patterns Issues related to pattern matching.

Comments

@lrhn
Copy link
Member

lrhn commented Mar 13, 2023

We currently do not allow a void-typed expression to be used in a context where its value is used (only a context of type void or dynamic, and dynamic was only allowed to not have to rewrite legacy code).

We should consider whether void is allowed to flow into pattern matches.
We are already doing something on some platforms, but we should be consistent and deliberate about it.

I'd say "hard no", and disallow (aka. make compile-time error):

  • Any switch expression with static type void.
  • Any if-case matched value expression with static type void.
  • Any pattern being type-checked with matched value type void, unless the pattern is _ (or any other non-checking, non-binding pattern which also wouldn't need to look up the value of a list or map at all). That's needed for var (x, _, z) = o as (int, void, int);.

The first two are really just expressions-in-position-where-value-is-used, and could be covered by current behavior.
The last one is new, because there is no expression the original program which has static type void. It only exists in the desugared version. Still, the is a value with static type void which is pushed into a pattern match, and that pattern match actually looks at the value (it binds it to a new variable, one not typed as void).

We an allow a pattern to bind a void value to another variable of type void or dynamic, but you really shouldn't.
I'd prefer to make the void protection stronger here, since it's new syntax that doesn't have to accommodate existing code.

  • Also disallow using void as the type of a binding pattern, void v, to introduce a void-typed variable. Just use _, because you are not supposed to look at the value later anyway.

And

  • Disallow void as the static type of an object pattern. (Also if doing so through a type alias, but T(:var hashCode) is OK, even if T ends up bound to void at runtime. This is just static checking.)

Even though the object pattern void() is technically not looking at the object, it's also not useful, and you should just use _.
Doing void(:var runtimeType) is just plain wrong, you're invoking members on a void typed value. So

(Now that we have _, we no longer need to allow you to assign void values to anything. Or at least, we won't when we allow _ as a parameter name.)

Example code:

typedef OQ = Object?;

void main() {
  void v;
  
  switch (v) {
  // Error: This expression has type 'void' and can't be used.
  //   switch (v) {
  //           ^
    case OQ(:var hashCode): print(hashCode);
  }
  if (v case Object? o) print(o.hashCode);
  // Error: This expression has type 'void' and can't be used.
  //   if (v case Object? o) print(o.hashCode);
  //       ^
  
  if ((1, v) case (int _, Object? o)) { // No error
    print(o.hashCode);
  }

  if (null case void v) { // Safe, but unnecessary
    print(v as dynamic);
  }
  
  if (null case Void(:var hashCode)) { // Reads `.hashCode` of `void`
    print(hashCode);
  }
}

This was run in dartpad.dev on master branch.
Dart2js rejects two of the cases, but not the third.
The analyzer gives no warnings whatsoever. (Should probably be considered a bug.)

@dart-lang/language-team

@lrhn lrhn added the patterns Issues related to pattern matching. label Mar 13, 2023
@eernstg
Copy link
Member

eernstg commented Mar 13, 2023

Sounds good!

I guess the missing part in the current ruleset is that the matched value type can be void, and the matched value type isn't the static type of any expression. (We could take desugaring into account, and use the fact that, most likely, every matched value type is the type of a local variable. But we don't want to depend on desugaring in the specification of the static analysis of patterns.)

I suspect that the following new rule would then suffice:

During type checking of a pattern (in a pattern declaration, pattern assignment, or in a refutable context), a compile-time error occurs if the matched value type is void, and the pattern is not a wildcard identifier pattern _, a non-matching rest pattern ..., or a cast pattern.

About the following rules:

  • Any switch expression with static type void.
  • Any if-case matched value expression with static type void.
  • Any pattern being type-checked with matched value type void, unless the pattern is _ (or any other non-checking, non-binding pattern which also wouldn't need to look up the value of a list or map at all). That's needed for var (x, _, z) = o as (int, void, int);.

As you mention, the first one is already partially covered: It is an error for an expression (including switch expressions, unless we specifically put them on the whitelist) to have type void, except in the case where it occurs in one of the whitelisted locations. For instance, it's not an error to pass it as an actual argument to a formal parameter of type void, and it's not an error to use it in an as expression. We could of course make it an error for the switch expression to have type void no matter where it occurs, but that's inconsistent with other expressions of type void, and I don't think that's particularly helpful.

The second bullet should be covered fully (by the rule I mentioned above, and also by the third bullet), because the initial matched value type for the pattern in that if-case will then be void.

Finally, I think _ and ... are obvious candidates as patterns whose matched value type can be void. The cast pattern is derived from whitelisting as expressions, and it is probably more controversial. We could of course try to limit these whitelisted patterns as much as possible, but it does seem inconsistent to allow myVoidExpression as Object, but not the corresponding as pattern.

@munificent munificent added patterns-later and removed patterns Issues related to pattern matching. labels Apr 4, 2023
@munificent
Copy link
Member

This isn't going to make it into 3.0, but I also mostly don't think it matters much one way or the other.

@Quijx
Copy link

Quijx commented Apr 21, 2023

I was just playing around the the new patterns an came across a problem, which I think is related.
Let's say I have the following code:

void main() {
  printFeature();
  printBugs(1);
}

void printFeature() => print('feature');

void printBugs(int n) => switch (n) {
      0 => print('no bugs'),
      1 => print('one bug'),
      _ => print('$n bugs'),
    };

This without the printBugs and it's call, this runs just fine, but with it, the following error is shown when compiling.

bin/void_bug.dart:9:12: Error: This expression has type 'void' and can't be used.
      0 => print('no bugs'),
           ^
bin/void_bug.dart:10:12: Error: This expression has type 'void' and can't be used.
      1 => print('one bug'),
           ^
bin/void_bug.dart:11:12: Error: This expression has type 'void' and can't be used.
      _ => print('$n bugs'),
           ^

However the analyzer does not see this as an error and no red lines are shown.

So at least the analyzer would have to report the error as well, but I think using a switch expression with the type void would still be useful for cases such as the one shown. That way the printBugs function does not need a block body. Also it would be consistent with the way we can do it in printFeature.

@lrhn
Copy link
Member Author

lrhn commented Apr 21, 2023

@Quijx
That's a separate issue. An expression with static type void (which print has) is only allowed in a number of specific positions, and the expression of a switch expression case is not among those. (Because we haven't explicitly added it to the list. Whether we'd want to, when the context type is void, is worth discussing, but I'd be against it.)

The analyzer apparently doesn't recognize that it's not allowed.

@munificent
Copy link
Member

There are no analyzer or CFE errors in:

void printBugs(int n) => n == 0
    ? print('no bugs')
    : n == 1
        ? print('one bug')
        : print('$n bugs');

Personally, I think it's reasonable to allow void-typed switch case expressions, and it's practically useful in examples like @Quijx's here. Sure, you could argue that a switch statement would be better, but we know empirically that users like using => for short method bodies even when the return type is void.

@lrhn
Copy link
Member Author

lrhn commented Apr 27, 2023

From dart-lang/sdk#52191

The language has some fairly ad hoc rules about where expressions of type void are and aren't allowed. The patterns specification makes no mention of disallowing expressions of type void anywhere so I assumed that, by default, they would be allowed.

I assumed, with the same reasoning, that they would be disallowed.
The list of places where a void-typed expression is allowed looks like an allow-list, so nothing else is allowed unless explicitly mentioned.

That said, tail position is usually where we allow void expressions, if they are in a void context.
(Or rather, we don't worry about the context, because a void in a tail position will make the surrounding expression have static type void too, and then we leave the decision about whether that's OK to the the outer expression and context.)
Expression switch case result expressions (need a better name here!) are in tail position.

My only worry is whether one switch case result having type void is enough to ensure that the entire switch has type void, after UP-ing all the branch types. (But that's probably just as safe as just the two branches of a conditional expression.)

So, LGTM allowing "expression switch case result expressions" to be void-typed.

@leafpetersen
Copy link
Member

My only worry is whether one switch case result having type void is enough to ensure that the entire switch has type void, after UP-ing all the branch types. (But that's probably just as safe as just the two branches of a conditional expression.)

I believe that the definition of UP that I cited in one of the issues talking about this guarantees this.

So, LGTM allowing "expression switch case result expressions" to be void-typed.

+1

@munificent
Copy link
Member

I assumed, with the same reasoning, that they would be disallowed.

🙃

This is a corner of the spec I'm not very familiar with and I didn't realize it was more like an allowlist.

@eernstg
Copy link
Member

eernstg commented Apr 28, 2023

I think we can do it like this: #3026.

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

No branches or pull requests

5 participants