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

Extension of IL verification rules to address readonly references #21

Merged
merged 4 commits into from
Jan 22, 2018

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Nov 6, 2017

A proposal for extension of IL verification rules that would allow operating with readonly references without defensive copying in verifiable code.

In the nutshell the rules would allow type-safe scenarios like fetching a reference to a readonly field and passing that to an “in” parameter be formally verifiable.
The proposal explains how “readonliness” demands and guarantees can be propagated and validated by the IL verification algorithm.

Copy link

@kumpera kumpera left a comment

Choose a reason for hiding this comment

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

Spec looks right and doable. We'll schedule work to prototype it on mono.

## Merging stack states ##
*(add to III.1.8.1.3 Merging stack states)*

Merging a readonly managed pointer with an managed pointer of an ordinary or controlled mutability kind results in a managed pointer of that kind.
Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I adopted the part of this rule from the "returnable ref" proposal, but in this case the readonly should win over since it is the most constrained.

Will fix. Thanks!!

@VSadov
Copy link
Member Author

VSadov commented Nov 18, 2017

After a review of the rules around implementing/overriding with @davidwrighton it has become clear that it is not always possible to guarantee that methods match in terms of readonly across the hierarchy.

Hence the change in that part of the proposal. We no longer rely on verification to match readonly in the inheritance/implementation chain.
Instead we codify the current use of modreq modifiers in the return positions and in parameter positions of virtual and abstract methods.

While so far the primary purpose of the modreq was to prevent misuse of readonly from old compilers. It appears to be naturally solving the problem with matching readonly when overriding/implementing. Thus the modreq are now required.

@VSadov
Copy link
Member Author

VSadov commented Nov 18, 2017

@davidwrighton - please take a look

@MichalStrehovsky
Copy link
Member

Would it make sense to try to align this with how C++/CLI handles const? C++/CLI set some precedent that other .NET languages (we don't know about) could have chosen to follow. .NET is not just about C#/VB/F#.

public value class A
{
public:
  const A% GetA(const A% a) { return a; }
};

Gets represented as this in the metadata:

.method public hidebysig instance valuetype A modopt([mscorlib]System.Runtime.CompilerServices.IsConst)& 
        GetA(valuetype A modopt([mscorlib]System.Runtime.CompilerServices.IsConst)& a) cil managed

Things of note:

  • IsConst is used as the modifier type. I know there's nothing wrong with using a custom attribute as proposed here, but this is less weird.
  • The modifier is optional. Do we want to think about that?
  • The placement of the modifier (modified is the type pointed-to)

@VSadov
Copy link
Member Author

VSadov commented Jan 22, 2018

Merging this, since this has been opened for a long time, and informally approved by interested parties.

NOTE: The proposal does not have direct impact on the language, but has effect on the compiler.
It codifies the metadata shape which has direct impact on interop/verification scenarios.
(that is why it fits in csharplang repo)

@VSadov VSadov merged commit 1a7fc72 into dotnet:master Jan 22, 2018
@MichalStrehovsky
Copy link
Member

Merging this, since this has been opened for a long time, and informally approved by interested parties.

@VSadov Were there reasons why we needed to reinvent the metadata encoding for this? (See how C++/CLI represents this above.)

(I've been labeled the owner of the metadata stack in the CLR for a while so I would say I'm an "interested party").

@VSadov
Copy link
Member Author

VSadov commented Jan 23, 2018

IsConst was the first thing we tried here. It turned out not possible - specifically because it is already in use for various purposes. And in particular because C++ knows about it and in many cases simply ignores it.
Changing compilers retroactively to understand IsConst as to mean what it means in C# would be breaking now.

The conclusion was - if we want something with defined semantics it must be something new or we run into issues where it already means something different from what we need.

I.E. ironically - if we had chosen IsConst we would close the door to interop with C++ as our use would conflict with theirs.

@VSadov
Copy link
Member Author

VSadov commented Jan 23, 2018

The proposal here is to rationalize rules how the metadata that C# already emits (with some additions around locals) could be used in verification.
There seem to be an agreement that it can and should be done.

It is still a proposal, but not a “proposal for a proposal” :)

@VSadov
Copy link
Member Author

VSadov commented Jan 24, 2018

@MichalStrehovsky - please take a look at #31

After thinking a bit about whether IsReturnable should be an attribute, I am getting convinced that it should be just a regular class like IsConst. Its only foreseeable target is IL local slots and that is not even expressible in AttributeTargets enum. - Making this type an attribute seems unnecessary and perhaps confusing.

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.

3 participants