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

Grammar rule *parameter_modifier* disallows valid V7 constructs #1209

Open
wants to merge 1 commit into
base: draft-v8
Choose a base branch
from

Conversation

RexJaeschke
Copy link
Contributor

[I stumbled on this today while looking at how to spec the addition of scoped to a parameter for a future version.]

The relevant grammar for method parameters in §15.6.2.1 is, as follows:

fixed_parameter
    : attributes? parameter_modifier? type identifier default_argument?
    ;

parameter_modifier
    : parameter_mode_modifier
    | 'this'
    ;

parameter_mode_modifier
    : 'ref'
    | 'out'
    | 'in'
    ;

Rule parameter_modifier allows parameter_mode_modifier or this, but not both. However, two combinations of these are permitted, as follows:

public static class E
{
    public static void F1b(this in int p) { }
    public static void F1d(this ref int p) { }
}

This possibility is supported by the text in §15.6.10 Extension methods, which states:

When the first parameter of a method includes the this modifier, that method is said to be an extension method. Extension methods shall only be declared in non-generic, non-nested static classes. The first parameter of an extension method is restricted, as follows:

  • It may only be an input parameter if it has a value type
  • It may only be a reference parameter if it has a value type or has a generic type constrained to struct
  • It shall not be a pointer type.

I propose the following grammar change:

parameter_modifier
    : parameter_mode_modifier
    | 'this' parameter_mode_modifier?
    ;

Now this new grammar also allows this out, which according to my compiler is not permitted [CS8328], and that suggests we also add to the extension bullet list above, the following constraint:

  • It shall not be an output parameter.

@RexJaeschke RexJaeschke added meeting: discuss This issue should be discussed at the next TC49-TG2 meeting meeting: proposal There is an informal proposal in the issue, worth discussing in a meeting labels Nov 17, 2024
@RexJaeschke RexJaeschke added this to the Pre-C# 8.0 milestone Nov 17, 2024
@RexJaeschke
Copy link
Contributor Author

Re my second suggestion, I just found the following in §15.6.2.1:

If the parameter is a struct type or a type parameter constrained to a struct, the this modifier may be combined with either the ref or in modifier, but not the out modifier. Extension methods are further described in §15.6.10.

That said, having the new bullet I proposed clarifies it at the parameter end of the call in the case of extension methods, which are not singled out by the above quote.

@@ -2130,7 +2130,7 @@ default_argument

parameter_modifier
: parameter_mode_modifier
| 'this'
| 'this' parameter_mode_modifier?
Copy link
Contributor

Choose a reason for hiding this comment

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

The ordering is flexible:

Suggested change
| 'this' parameter_mode_modifier?
| 'this' parameter_mode_modifier?
| parameter_mode_modifier? 'this'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My test shows that the order is indeed flexible. Given Nigel's proposal, this on its own would be ambiguous, but I think ANTLR chooses the lexical-first rule that matches, right, Nigel?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RexJaeschke – Yes, ANTLR resolves such ambiguity by picking the first alternative. We have noted elsewhere where the grammar relies on this, however in this case as the two alternatives represent exactly the same thing I don’t think a note is necessary.

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

Requires a change to the grammar, apart from that the changes are good.

In the preexisting text I did notice that a reference parameter can be of a generic type constrained to a struct, an input parameter cannot be, and there appears to be no explanation why. The answer can be found in the Readonly references proposal, should something (normative or informative) be added?

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Can we rename parameter_modifier to parameter_modifiers? It doesn't feel like this ref is a single parameter modifier. This is purely a stylistic thing, and I'm happy to be overruled.

@Nigel-Ecma
Copy link
Contributor

Can we rename parameter_modifier to parameter_modifiers? It doesn't feel like this ref is a single parameter modifier. This is purely a stylistic thing, and I'm happy to be overruled.

I'll vote against on this one – not because I think the current naming in this area is good, it isn’t really, but because making it plural won’t make it any better so stick with the status quo.

@jskeet jskeet modified the milestones: Pre-C# 8.0, C# 8.0 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting meeting: proposal There is an informal proposal in the issue, worth discussing in a meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants