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

Proposed changes to ECMA 335 for ref field support #63659

Merged
merged 6 commits into from
Feb 16, 2022

Conversation

davidwrighton
Copy link
Member

In support of #32060

These changes allow definition and access to ref fields

@ghost
Copy link

ghost commented Jan 12, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

In support of #32060

These changes allow definition and access to ref fields

Author: davidwrighton
Assignees: -
Labels:

area-Meta

Milestone: -

@davidwrighton
Copy link
Member Author

@jaredpar Could you assign someone from Roslyn to look at these changes and provide comments?
@mangod9 Manish, fyi.
@AaronRobinsonMSFT Please take a look. Once we agree on the ECMA changes we can see what code can be pulled from the hackathon branch to implement/test all of this.

@davidwrighton davidwrighton changed the title Initial proposed changes to ECMA 335 for ref field support Proposed changes to ECMA 335 for ref field support Jan 12, 2022
docs/design/specs/Ecma-335-Augments.md Outdated Show resolved Hide resolved
- Replace “Typed references have the same restrictions as byrefs.” With “Typed references are byref-like types and have the same restrictions as normal byref-like types.”
- Replace “They can be used for local variable and parameter signatures. The use of these types for fields, method return types, the element type of an array, or in boxing is not verifiable (§I.8.8). These two types are referred to as byref-like types.” With “If a function uses these types only for local variable and parameter signatures, then the use of those types is verifiable. Otherwise these types have the same restrictions as normal byref-like types.”
### I.8.6.1
- Remove “(A fifth kind, a local signature (see §I.8.6.1.3) is really a version of a location signature.)”
Copy link
Member

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.

Copy link
Member Author

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.

- Remove “In addition, there is one special local signature.”
- Remove “A typed reference is a full signature in itself and cannot be combined with other constraints. In particular, it is not possible to specify a byref whose type is typed reference.”
- Replace “This type shall only be used for parameters and local variables. It shall not be boxed, nor shall it be used as the type of a field, element of an array, or return value.” With “A typed reference is considered to be a by-reflike structure”
- Remove “A typed reference is a full signature in itself and cannot be combined with other constraints. In particular, it is not possible to specify a byref whose type is typed reference.” From the third paragraph
Copy link
Member

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.

Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Jan 12, 2022

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).

Copy link
Member

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't CLSCompliant and there are several cases, including where 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.

Copy link
Member

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.

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.

Copy link
Member

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.

docs/design/specs/Ecma-335-Augments.md Show resolved Hide resolved
@teo-tsirpanis
Copy link
Contributor

Are there plans for a seventh version of ECMA 335 BTW?

@KalleOlaviNiemitalo
Copy link

II.9.4 already says that "Byref-like types" cannot be used as type arguments, so it's OK that this PR does not specify that restriction.


### I.8.2.1.1 Managed pointers and related types
- First paragraph. Replace “Managed pointer types are only allowed for local variable (§I.8.6.1.3) and parameter signatures (§I.8.6.1.4); they cannot be used for field signatures (§I.8.6.1.2), as the element type of an array (§I.8.9.1), and boxing a value of managed pointer type is disallowed (§I.8.2.4).” with “Managed pointer types are only allowed for local variable (§I.8.6.1.3), parameter signatures (§I.8.6.1.4), and instance fields of byref-like types; they cannot be used for field signatures(§I.8.6.1.2) of static fields, or of instance fields of types which are not byref-like, as the element type of an array (§I.8.9.1), and boxing a value of managed pointer type is disallowed (§I.8.2.4).”
- Add a new paragraph before the paragraph on the three special types. “Byref-like types are value types which may contain managed pointers, or pointers onto the VES stack. Byref-like types have the same restrictions as byrefs. Value types which are marked with the System.Runtime.CompilerServices.IsByRefLikeAttribute attribute are considered to be byref-like types.”

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.

Copy link
Member Author

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.

@jaredpar
Copy link
Member

@AlekseyTs, @RikkiGibson, @cston FYI as this will relate to the ref fields support we are considering in the language


### I.8.6.1.2
- Move the contents of the byref constraint paragraph from Section I.8.6.1.3 to a bullet point.
“- The byref constraint states that the content of the corresponding location is a managed pointer. A managed pointer can point to a local variable, parameter, field of a compound type, or element of an array. However, when a call crosses a remoting boundary (see §I.12.5) a conforming implementation can use a copy-in/copy-out mechanism instead of a managed pointer. Thus programs shall not rely on the aliasing behavior of true pointers. A managed pointer cannot point to another managed pointer, but a managed pointer can point to a byref-like type.”
Copy link
Member

Choose a reason for hiding this comment

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

A managed pointer cannot point to another managed pointer,

This is effectively saying that ref ref int is not supported at an IL level correct?

Copy link
Member

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.

Copy link
Member

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 have ref ref by having ref Span<T> or really any ref SomeTypeWithRefFields. Hence wanted to understand a bit more why ref ref is disallowed because it seems like it's effectively allowed with a bit of indirection.

Copy link
Member

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.

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 if ref 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 exclude ref ref types even without having the spec spell it out.

Copy link
Member Author

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.

  • Mono would need to restructure its representation of type
  • We would need to update the type name generation/parsing for custom attribute types
  • A bunch of little implementation details scattered all throughout the CoreCLR runtime would need updates.
  • We would need new syntax in C#, or there may be a whole set of unspeakable types that can only appears as a result of generics or something
  • @KalleOlaviNiemitalo also describes that remoting is a thing that would be impacted. At this time, that is not a major concern, but he is unquestionably correct.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Mono would need to restructure its representation of type

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

@@ -948,3 +949,47 @@ Conversions from floating-point numbers to integral values truncate the number t
on the top of the stack is reinterpreted as an unsigned value before the conversion.
Note that integer values of less than 4 bytes are extended to int32 (not native int) on the
evaluation stack.

## Ref Fields
To improve the usefulness of ref structs, support for fields which are defined as byrefs is needed. Currently their functionality can be approximated by Span<T> fields, but not all types can be converted into Span<T> types simply. In order to support these customers support for generalizing byref fields, and converting TypedReference, ArgIterator and RuntimeArgumentHandle into following the normal rules of C# ref structs is desired. With this set of changes, it becomes possible to have ByRef fields of T, but support for pointers to ByRef fields or ByRefs to ByRefs is not added to the ECMA specification.
Copy link
Member

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>* and ref 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?

Copy link
Member Author

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.

  • Mono would need to restructure its representation of type
  • We would need to update the type name generation/parsing for custom attribute types
  • A bunch of little implementation details scattered all throughout the CoreCLR runtime would need updates.
  • We would need new syntax in C#, or there may be a whole set of unspeakable types that can only appears as a result of generics or something

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.

@davidwrighton
Copy link
Member Author

Are there plans for a seventh version of ECMA 335 BTW?

No. There is no current plan to fund such an effort, but we do make an effort to record the differences from the current spec so that should we want to do that in the future, we are able to produce a good set of updates to work from.

@ghost
Copy link

ghost commented Feb 6, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

In support of #32060

These changes allow definition and access to ref fields

Author: davidwrighton
Assignees: davidwrighton
Labels:

area-System.Runtime

Milestone: -

@davidwrighton davidwrighton merged commit 135a960 into dotnet:main Feb 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
@davidwrighton davidwrighton deleted the ecma_changes_for_ref_fields branch April 13, 2023 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants