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

Create spec for const expressions for is patterns #7589

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

Conversation

Rekkonnect
Copy link
Contributor

@Rekkonnect Rekkonnect commented Oct 9, 2023

Spec for #6926

CC @333fred

@Rekkonnect Rekkonnect requested a review from a team as a code owner October 9, 2023 23:44

## Unresolved questions
[unresolved]: #unresolved-questions

- [ ] Requires LDM review
- [ ] Should we introduce a new error for non-constant subpatterns in order to isolate the root cause of the inability to consider the expression constant?
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a language concern. The compiler is free to make most-specific errors if it can and it would be helpful. It doesn't need to be specified.


### Semantics
[semantics]: #semantics
A pattern expression only consisting of the above subpatterns with a constant LHS may be evaluated at compile time currently, but it cannot be assigned to a constant symbol. With this change, constant fields and locals of type `bool` may be assigned pattern expressions, as already evaluated during compilation.
Copy link
Member

Choose a reason for hiding this comment

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

This is overly specific. For example, it would prevent this code, which I would expect to be valid:

const int i = (1 is 2) ? 3 : 4;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't intend to exclude support for this valid expression, and I think the document doesn't imply the inability to use constant pattern expressions in the evaluated expression of a conditional operator

Copy link
Member

Choose a reason for hiding this comment

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

the wording says:

With this change, constant fields and locals of type bool may be assigned pattern expressions,

Which indicates, only fields/locals could benefit from this. My recommendation would be to go to the part of the spec on constant evaluation and just update it as necessary. The cases you want will fall out. By adding these extra clauses, it seems to be adding more restrictive requirements.

## Motivation
[motivation]: #motivation

Certain pattern expressions that can be converted into simpler boolean expressions using equality and comparison operators are never considered constant expressions, even when all parts of the expression are considered constant and can be evaluated during compilation. However, the lowered versions of those expressions (that only involve equality and comparison operators) are always considered constant. This prohibits the ability to utilize `is` expressions for operations like comparing against a range (e.g. `x is >= 'a' and 'z'`), or checking against distinct values (e.g. `x is Values.A or Values.B or Values.C`).
Copy link
Member

Choose a reason for hiding this comment

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

like comparing against a range (e.g. x is >= 'a' and 'z')

i'm confused by this statement. 'is' is not prohibited in either of these cases. Both are legal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad wording, I meant that we are not currently able to use this kind of syntax for that purpose, and use that result as a const expression. Will update to make it clearer.

> - sizeof expressions, provided the unmanaged-type is one of the types specified in §23.6.9 for which sizeof returns a constant value.
> - Default value expressions, provided the type is one of the types listed above.

The allowed subpatterns as shown in the list above are called "constant subpatterns", as they will be eligible for compile-time evaluation.
Copy link
Member

Choose a reason for hiding this comment

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

presumably the not, and and or patterns should also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And parenthesized ones too? I didn't initially consider them to be separate kinds of patterns.

Copy link
Member

Choose a reason for hiding this comment

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

i'd enumerate the exact set of patterns we expect this to work for (same as how we enumerate the exact set of unary and binary operators, etc.). note: based on how the constant-expressions spec works out, i think everything else will likely fall out.

You can then give examples for each case. I don't htink you have to specify how it evaluates as the constant expression section already says this:

Whenever an expression fulfills the requirements listed above, the expression is evaluated at compile-time. This is true even if the expression is a subexpression of a larger expression that contains non-constant constructs.

The compile-time evaluation of constant expressions uses the same rules as run-time evaluation of non-constant expressions, except that where run-time evaluation would have thrown an exception, compile-time evaluation causes a compile-time error to occur.

--

Note: @333fred if we're doing this spec, would it makes sense to roll in switch expressions at the same time, given that the lang already supports ?:? Or would you prefer to keep that excluded? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

LDM did express interest in it, but didn't directly approve it. Probably best to leave it as an open question for now.

Copy link
Member

Choose a reason for hiding this comment

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

That works for me. We could bring for an LDM read and easily add if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, there is a discussion for the equivalent extension to support switch expressions in #7489, which will also be noted in the spec doc

Comment on lines 62 to 72
> - **Boolean literal patterns.**
> - **Numeric literal patterns.**
> - **Character literal patterns.**
> - **String literal patterns.**
> - **Relative numeric literal patterns.**
> - **Relative character literal patterns.**
> - **Null literal patterns.**
> - **Default literal patterns.**
> - **And, or and not patterns.**
> - **Parenthesized patterns.**
> - **References to constant fields or locals inside constant patterns.**
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the word literal from these? It doesn't seem to be necessary, given the recursive nature of "constant expressions must be composed of other constant expressions", and it will reduce the brittleness here for future changes.

proposals/pattern-const-expressions.md Outdated Show resolved Hide resolved
Comment on lines 118 to 123
When assigning those expressions in a constant context, these warnings about the constant result of the expression will **not** be reported, as the user intends to capture the constant value of the expression. Constant context involves:
- initializers for const symbols (fields or locals)
- attribute arguments
- parameter default value
- switch statement case labels
- switch expression arms
Copy link
Member

Choose a reason for hiding this comment

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

We need to see the actual spec changes here. How do we have to redefine the sections that specify these constructs to introduce a constant context? What is the definition of a constant context itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated ptal


When assigning those expressions in a constant context, these warnings about the constant result of the expression will **not** be reported, as the user intends to capture the constant value of the expression.

Constant context includes contexts where a constant expression is required, as indicated in the [§12.23 section](https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/expressions.md#1223-constant-expressions), specifically:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Constant context includes contexts where a constant expression is required, as indicated in the [§12.23 section](https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/expressions.md#1223-constant-expressions), specifically:
Constant contexts are contexts where a constant expression is required, as indicated in the [§12.23 section](https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/expressions.md#1223-constant-expressions), specifically:

> - Attributes ([§22](attributes.md#22-attributes))
> - In a *constant_pattern* ([§11.2.3](patterns.md#1123-constant-pattern))

Based on the above, we only filter constant contexts that accept `bool` values, namely:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this means?

- `goto case` statements
- Switch expression arms

Examples for the above:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some more examples. All of the examples here are always true, we want some always false as well. Let's also throw in some switch expression examples with proposed behavior; I don't want to go through the whole specification of switch expressions yet, but we can at least include some examples to demonstrate warning behavior for unreachable arms/locations where the arms don't cover the whole input range (ie, when you cover case 1 and 2, but not 3-uint.MaxValue).

@Rekkonnect
Copy link
Contributor Author

I believe we should also support tuple pattern matching expressions of the form (x, y) is (a, b), when x and y are also constants. This tuple syntax does not create a tuple value, but instead deconstructs the expression into two comparisons of the two values, equivalent to x is a && y is b. This can be evaluated in compile time, and thus is eligible for being supported from this change.

I did not add the above to the spec before discussing the idea, since it deviates from the classical concept of a constant expression by being primarily driven by syntactical simplification.

@333fred
Copy link
Member

333fred commented Jan 5, 2024

I believe we should also support tuple pattern matching expressions of the form (x, y) is (a, b), when x and y are also constants.

I don't think I'm willing to champion this. These are not constants today, and will need a good deal of language and compiler changes to implement. Let's keep the proposal to just the things that have been championed.

### Switch expressions
[switch-expressions]: #switch-expressions

When evaluating a constant value on a switch expression, we adjust the reported diagnostics based on the matching arms:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem quite right, and LDM was concerned by the specific behavior that you show examples of below. We wanted to limit the subsumption changes to only constant contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it fine if we make raise the warning into an error if we fail to match any subpattern in the constant switch? The constant value cannot be assigned without being provided a value from the default case, should all others fail to match.

For example,

const int a = 4;
// ERROR: No subpattern is matched, and the assigned value is a constant
const int c = a switch
{
    1 => 2,
    2 => 3,
    3 => 5,
};

// WARNING: No subpattern is matched, the default case will be handled and it will throw at runtime.
int v = a switch
{
    1 => 2,
    2 => 3,
    3 => 5,
};

I believe that both of the above cases should be an error, but I'd like your input about those.

Copy link
Member

Choose a reason for hiding this comment

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

The second case should have unchanged behavior from today. The first case would indeed be an error.

Copy link
Member

Choose a reason for hiding this comment

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

In general, the wording itself is not what I expected here. I don't see any mention of constant contexts, which is what I am expecting to drive changes in warning behavior.

const int c = 3;

// no warning about the missing default arm,
// the expression always returns the value of b
Copy link
Member

Choose a reason for hiding this comment

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

LDM was specifically interested in making this not change behavior.

// switch expression arm
var p = b switch
{
// always true, no warning/error
Copy link
Member

Choose a reason for hiding this comment

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

2 problems with this:

  1. This is actually missing a case. b is false, and a is c is true.
  2. The overall switch expression should not change behavior, which is to say it should still warn about missing cases.

};

// we get a warning about the missing default arm,
// the expression results in xc being assigned the value of b
Copy link
Member

Choose a reason for hiding this comment

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

I would not expect to get a warning here.

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.

4 participants