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

Ref safety: replace uint scope with struct SafeContext #75647

Merged
merged 15 commits into from
Nov 8, 2024

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Oct 26, 2024

Update: renamed the new type to SafeContext to match the language standard. #75647 (comment)


The 'struct Lifetime' represents a shift in philosophy toward a type-like solution for ref safety. Rather than directly using uints, equality/relational operators, Math.Min/Max, etc. we express things in terms of "convertibility" of these semi-opaque Lifetime values as well as 'Intersect' and 'Union' operators. Hopefully you will find that this improves the clarity of the code quite a bit.

I am adopting the term "lifetime" rather than "escape scope", "safe to escape", "ref-safe-scope", etc. very deliberately here. I am coming around to the opinion that using the term "lifetime" is much clearer than any of the above.

Separately, I am hoping to publish a spec for this area which will hopefully demonstrate that improvement in clarity. That spec will probably be an expansion upon low-level-struct-improvements.md#annotations, but as a standalone document. The goal of that document would be to succinctly and fully define all the ref safety rules in the language, and provide a basis for deciding if any given program is "ref-safe".

Because the dynamics of how ref lifetimes can be nested and related is much, much simpler than for types in C#, you can think of this as more of a metaphor for how things work than anything else. We are not "reifying" the concept of a lifetime as a type, we are not appending this concept to the symbol model in any way, etc. In the compiler it's just a "temporary" concept which is introduced for the purpose of these safety checks and diagnostics and then dropped.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 26, 2024
@RikkiGibson RikkiGibson marked this pull request as ready for review October 26, 2024 20:55
@RikkiGibson RikkiGibson requested a review from a team as a code owner October 26, 2024 20:55
@RikkiGibson
Copy link
Contributor Author

@cston @jjonescz @jaredpar for review.

(I think there are other PRs modifying this code at the same time. This one can wait till after they go in and handle the conflicts.)

@jcouv jcouv self-assigned this Oct 30, 2024
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 8). The change looks good and helps readability. Some minor nits to consider. Thanks

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 10)

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Nov 7, 2024

Based on offline discussion and investigation, I am thinking this is not the right time or order to introduce a new term. Terms which we want to match specification, should be introduced first in specification. ECMA has written quite a bit already about these features in terms of safe-context, so, backing that train up also requires a proportional level of justification and work.

The goal of this PR is to simply improve understandability of this area. So let's do that on a standalone basis for now. Will update PR title prior to merging.

@jaredpar
Copy link
Member

jaredpar commented Nov 7, 2024

Update: renamed the new type to SafeContext to match the language standard

Semes like the safe choice in this context

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs Outdated Show resolved Hide resolved
/// </summary>
public SafeContext Wider()
{
Debug.Assert(_value >= CurrentMethodRaw);
Copy link
Member

Choose a reason for hiding this comment

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

Why CurrentMethodRaw vs. ReturnOnly? It should be valid to get a wider scope from a ReturnOnly context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of the method is to "exit" a local scope, alongside Narrower() to "enter" a local scope. It's not intended to, for example, transition between the well-known "returnable" scopes. (We do allow this method to return ReturnOnly, though, to allow for uniformity when exiting the "top-level" block of a method in the visitor.)

Co-authored-by: Jared Parsons <jared@paranoidcoding.org>
@RikkiGibson
Copy link
Contributor Author

It looks like following tests are failing in Release_32, which appear unrelated to my changes:

  • Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.VisualBasicCompletionCommandHandlerTests.TestMatchWithTurkishIWorkaround9
  • Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.VisualBasicCompletionCommandHandlerTests.TestMatchWithTurkishIWorkaround10

@RikkiGibson RikkiGibson changed the title Ref safety: replace uint scope with struct Lifetime Ref safety: replace uint scope with struct SafeContext Nov 8, 2024
@RikkiGibson RikkiGibson merged commit 46909e9 into dotnet:main Nov 8, 2024
21 of 24 checks passed
@RikkiGibson RikkiGibson deleted the uint-to-lifetime branch November 8, 2024 19:01
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 8, 2024
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants