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.

Sign up for GitHub

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

Jump to bottom

Add Lock object feature #71716

Merged
merged 16 commits into from
Feb 7, 2024
Merged

Add Lock object feature #71716

merged 16 commits into from
Feb 7, 2024

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jan 19, 2024 •
edited
Loading

Test plan: #71888
Speclet: https://github.com/dotnet/csharplang/blob/main/proposals/lock-object.md

  • Lowers lock on the new Lock type to using.
  • Adds warning when upcasting Lock to object/dynamic/interface/base type.
  • Checks for Lock.Scope ref struct constraints - it cannot be used in using in async method.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 19, 2024
@runfoapp runfoapp bot mentioned this pull request Jan 19, 2024
Closed
@jjonescz jjonescz marked this pull request as ready for review January 19, 2024 19:23
@jjonescz jjonescz requested a review from a team as a code owner January 19, 2024 19:23
@jjonescz jjonescz requested review from RikkiGibson and cston January 23, 2024 08:39
AlekseyTs
AlekseyTs reviewed Jan 23, 2024
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs

namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Semantics;

public class LockTests : CSharpTestBase
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

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

LockTests

We should add tests for new scenarios to IOperationTests_ILockStatement.cs. #Pending

AlekseyTs
AlekseyTs reviewed Jan 23, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_LockStatement.cs
/// before and after the body, respectively.
/// Lowers a lock statement to a try-finally block that calls (before and after the body, respectively):
/// <list type="bullet">
/// <item>Lock.EnterLockScope and Lock+Scope.Dispose if the argument is of type Lock, or</item>
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

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

Lock.EnterLockScope and Lock+Scope.Dispose if the argument is of type Lock, or<

It looks like ControlFlowGraphBuilder.VisitLock should be adjusted to support the new form of lowering. #Pending

@AlekseyTs
Copy link
Contributor

It doesn't look like support for VB is implemented. It feels like we would want VB to recognize the new type as well, one way or another.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 24, 2024 •
edited
Loading

We should check if SemanticModel has any lock statement specific APIs. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1), new tests are not looked at.

@jjonescz jjonescz changed the title Add native lock feature Add Lock object feature Jan 31, 2024
@jjonescz jjonescz changed the base branch from features/NativeLock to features/LockObject January 31, 2024 13:55
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 11)

@jjonescz
Copy link
Member Author

jjonescz commented Feb 1, 2024

I haven't found any lock statement specific semantic model APIs.

In reply to: 1907138198

AlekseyTs
AlekseyTs approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 15). The remaining comments can be followed up on separately.

@jjonescz jjonescz requested a review from cston February 1, 2024 17:23
@jjonescz
Copy link
Member Author

jjonescz commented Feb 2, 2024

@cston @RikkiGibson for a second review, thanks.

@jjonescz jjonescz requested a review from cston February 6, 2024 10:27
@AlekseyTs
Copy link
Contributor

LGTM (commit 16)

@jjonescz jjonescz merged commit f105f5b into dotnet:features/LockObject Feb 7, 2024
25 checks passed
@jjonescz jjonescz deleted the NativeLock-01 branch February 7, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@cston cston cston approved these changes

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

Assignees
No one assigned
Labels
Area-Compilers Feature - Lock Type untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjonescz @AlekseyTs @cston @jcouv