Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposed changes to ECMA 335 for ref field support #63659
Proposed changes to ECMA 335 for ref field support #63659
Changes from 2 commits
f48a8c7
e113880
64999e3
5bb1c4f
b347660
5655ada
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean in the context of things like
Span<T>*
andref Span<T>
, both of which are legal today?Does it just mean you could only declare it "indirectly" (e.g.
ref SomeStructWithRefField
) or is there some other "subtlety" here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you could only declare it indirectly. There are a bunch of weird and unfortunate costs to adding the ability to take a ref of a ref, or a pointer to a ref.
Given that, I'd like to make the first effort to implement this simply not allow that more complex feature to exist. I don't currently see a need for it in the motivation for this feature. Also, given that is just a strict restriction on behavior, if it turns out to be a capability we want to add in the future, we can do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean only a System.Runtime.CompilerServices.IsByRefLikeAttribute type defined in the runtime, or also identically named types in user assemblies? IIRC, the spec does not require the runtime to recognize any other types just by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KalleOlaviNiemitalo We no longer attempt to update Partition IV of the ECMA specification, so we cannot refer to that as part of this set of updates, and the CoreCLR/Mono runtimes are likely to allow attributes defined in any module to satisfy this requirement, but I do not have any intention of mandating in the spec support for attributes by name only. (Parsing of attributes by name will be an implementation detail of the CoreCLR/Mono implementations). If you have suggestions for appropriate wording to clarify that, I'd appreciate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is removed. At the very least it should also mention there is a 5th signature given it is defined below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to remove the concept of local signatures, but upon review, it appears you are correct. I didn't quite do so. I'll try to rework this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is effectively saying that
ref ref int
is not supported at an IL level correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is also how I read this statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider that once we have
ref
fields then we can effectively haveref ref
by havingref Span<T>
or really anyref SomeTypeWithRefFields
. Hence wanted to understand a bit more whyref ref
is disallowed because it seems like it's effectively allowed with a bit of indirection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: I don't have any particular use for
ref ref
and no intention to add it to the language. But do want to understand why it's disallowed at the IL level when it seems like you can achieve it a slight amount of indirection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECMA-335 still partially describes remoting, although the .NET Runtime does not support remoting. I would prefer that the ECMA-335 augments be consistent with the remoting clauses until those clauses are deleted.
I.8.2.1.1 and I.8.6.1.3 mention a copy-in/copy-out mechanism for remoting of managed pointers. It is not clear what this should mean for
ref ref int
; just copying the bits of the managed pointer in and out seems too risky. So ifref ref
were allowed, then perhaps the spec should explicitly say that parameters with such types cannot be used across a remoting boundary.OTOH, the spec does not say that the
serializable
attribute affects remoting, nor that e.g. RuntimeArgumentHandle cannot be passed over a remoting boundary. Which means the implementation can choose which types it supports in remoting and can excluderef ref
types even without having the spec spell it out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you could only declare it indirectly via a structure. There are a bunch of weird and unfortunate costs to adding the ability to take a ref of a ref, or a pointer to a ref.
Given that, I'd like to make the first effort to implement this simply not allow that more complex feature to exist. I don't currently see a need for it in the motivation for this feature. Also, given that is just a strict restriction on behavior, if it turns out to be a capability we want to add in the future, we can do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a total dealbreaker (I prototyped it during the hackathon) but it will significantly increase implementation (really... testing and validation) cost for Mono in the .NET 7 timeframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I have no known immediate uses for it. Mostly I just wanted to understand why it wasn't directly allowed when it seems to exist perfectly fine with a small amount of indirection. The rest of your comment hit on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spec under "CLS Rule 14: Typed references are not CLS-compliant", the phase "CLS (framework): This type shall not appear in exported members." seems like it should be softened. I'd assume this type may start to appear in other types, no? I could be misreading what this is stating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just mark those members as not CLS-compliant, like any members whose type is UInt32 or an unmanaged pointer type (CLS rule 17).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be great if we could just remove the concept of
CLSCompliant
.C# already doesn't correctly warn everywhere it should (things like
modreq
aren'tCLSCompliant
and there are several cases, includingwhere T : unmanaged
, where C# doesn't force you to add the attribute).I, personally, view
CLSCompliant
a lot like "integers don't have to be two's complement representation" in C/C++. From an extensibility point it "makes sense" and allows the language/framework to "target more". However, from a practicality standpoint its a feature that basically doesn't get used and causes pain/headaches and various compat issues because half the tooling doesn't consider it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of
CLSCompliant
doesn't really exist on .NET Core. It's not really been a part of the tooling plan. Agree we should think about overall deprecating the concept if possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLS compliance is really only about API design guidelines.
The value of CLS compliance was discussed multiple times during API review meetings. One of the last discussions - #44194 (comment). I believe that the prevalent opinion in the room was that there is still some value in it and we are not ready to make the call to deprecate the concept.
I have tried to delete CLS compliant attributes in .NET Core to make it not really exist on. With the current Roslyn behavior, it would be a source breaking change for everybody who marks their assembly as CLS compliant. The compiler warns or errors when CLS compliant assemblies depend on non-CLS compliant assemblies.