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

Consider relaxing keyword ordering requirements with readonly ref (struct declaration) vs. ref readonly (return type declaration) #1530

Closed
stakx opened this issue May 16, 2018 · 9 comments

Comments

@stakx
Copy link
Contributor

stakx commented May 16, 2018

Earlier today I was writing something like this:

public readonly ref struct S { }

class T
{
    ref readonly S M(in S s)
    {
        return ref s;
    }
}

Being not yet very familiar with all the new ref toys, it took me a while to figure out the right ordering(s) of the keywords ref and readonly. It felt like I had to run into a wall a few times until I figured out the correct syntax (at least according to the Roslyn implementation):

  • In a struct declaration, only the ordering readonly ref is allowed.
  • In a method return type declaration, only the ordering ref readonly is allowed.

I can't say that I find this particularly intuitive. Of course I'll get used to this very quickly, but enforcing different orderings seems to make things harder than they really need to be.

  • Is there any specific reason why different keyword order was chosen in these two contexts?
  • Would it be possible to relax the current ordering rules in one or both contexts, so that both ref readonly and readonly ref would be allowed?
@svick
Copy link
Contributor

svick commented May 16, 2018

I think this is especially annoying, since the error messages are not very helpful:

error CS1031: Type expected

error CS0106: The modifier 'readonly' is not valid for this item

@HaloFour
Copy link
Contributor

#946 touches on this a little bit, although I can't tell if it's looking to relax the order of these modifiers for methods or just types.

@Korporal
Copy link

Korporal commented May 16, 2018

C# is indeed irritating when it comes to the ordering of keywords but I suspect this a price paid when committing to a C-like syntax. Also why "fix" this for one scenario? ideally every ordering scenario should be made more flexible not only readonly ref.

I recall the PL/I language (and a compiler I created for it) had no such restriction on keyword ordering but the fundamental syntax was designed to support this and I do not think C#'s C-like syntax (which was and is very primitive) can cope with this, you'd probably have to change the grammar.

Incidentally PL/I had no reserved words either.

More helpful diagnostics would be neat however in which the diagnostic message hinted at a possible ordering problem rather than the blunt message we do see.

@ufcpp
Copy link

ufcpp commented May 16, 2018

see https://github.com/dotnet/csharplang/blob/5cb84f4a66fd27dcd13395b12870f93e232ec3c5/meetings/2017/LDM-2017-02-22.md

Currently, refs themselves are always single-assignment in C#, but if we decide at some point to make them reassignable by default (which we could do without breaking), then you might want to be able to explicitly put a readonly modifier on the refs themselves. That would be readonly ref. And of course you could have readonly ref readonly where both the ref and the target are readonly.

@svick
Copy link
Contributor

svick commented May 16, 2018

@ufcpp Not sure how much of that is still relevant, since ref reassignment was introduced in C# 7.3, but readonly refs weren't.

@alrz
Copy link
Contributor

alrz commented May 16, 2018

If we had readonly locals readonly ref would mean something different (not for methods though).

But I agree the compiler could do a better job at reporting errors in this case.

@ufcpp
Copy link

ufcpp commented May 16, 2018

Though readonly ref is not implemented now, it could be in the future.

@stakx
Copy link
Contributor Author

stakx commented Jul 22, 2019

I'm going to close this issue. This repo has enough stale issues as it is, and this particular one has probably joined that club in the meantime.

@stakx stakx closed this as completed Jul 22, 2019
@alrz
Copy link
Contributor

alrz commented Jul 22, 2019

This is already championed. See #946

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

No branches or pull requests

6 participants