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

Implement control flow graph for Lock object feature #71987

Merged
merged 20 commits into from
Feb 15, 2024
Merged

Implement control flow graph for Lock object feature #71987

merged 20 commits into from
Feb 15, 2024

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Feb 7, 2024 •
edited
Loading

Test plan: #71888

  • This PR also changes the VB warning (when using Lock in VB's SyncLock statement) to be an error. That's better because we can potentially change the VB behavior in the future without breaking users. And thanks to that we can build the same control flow graph for C# and VB (since VB usage of Lock is an error anyway).

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 7, 2024
@jjonescz jjonescz marked this pull request as ready for review February 7, 2024 12:40
@jjonescz jjonescz requested a review from a team as a code owner February 7, 2024 12:40
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1).

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 12, 2024 •
edited
Loading 

        {

Consider asserting that forLock is false in this block #Closed

Refers to: src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs:3962 in 448700c. [](commit_id = 448700c, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 17)

AlekseyTs
AlekseyTs approved these changes Feb 13, 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 17), assuming CI is passing.

333fred
333fred reviewed Feb 13, 2024
View reviewed changes
src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_ILockStatement.cs
}

[Fact, CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)]
public void LockFlow_LockObject_Conditional_Object()
Copy link
Member

@333fred 333fred Feb 13, 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.

We should also have tests with conditional flow inside the body of the lock itself. #Resolved

333fred
333fred reviewed Feb 13, 2024
View reviewed changes
src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs Outdated
Comment on lines 152 to 153
if (!(scopeType is INamedTypeSymbol { Name: WellKnownMemberNames.LockScopeTypeName, Arity: 0, IsValueType: true, IsRefLikeType: true, DeclaredAccessibility: Accessibility.Public } &&
lockType.Equals(scopeType.ContainingType, SymbolEqualityComparer.ConsiderEverything)))
Copy link
Member

@333fred 333fred Feb 13, 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.

Please either do the DeMorgan's expansion, or indent the second line of this if so it doesn't imply that the Equals call is outside the parens.

Suggested change
if (!(scopeType is INamedTypeSymbol { Name: WellKnownMemberNames.LockScopeTypeName, Arity: 0, IsValueType: true, IsRefLikeType: true, DeclaredAccessibility: Accessibility.Public } &&
lockType.Equals(scopeType.ContainingType, SymbolEqualityComparer.ConsiderEverything)))
if (scopeType is not INamedTypeSymbol { Name: WellKnownMemberNames.LockScopeTypeName, Arity: 0, IsValueType: true, IsRefLikeType: true, DeclaredAccessibility: Accessibility.Public } ||
!lockType.Equals(scopeType.ContainingType, SymbolEqualityComparer.ConsiderEverything))
``` #Resolved

333fred
333fred reviewed Feb 13, 2024
View reviewed changes
src/Compilers/Core/Portable/Symbols/WellKnownMemberNames.cs
Comment on lines +402 to +404
internal const string LockTypeName = "Lock";
internal const string EnterLockScopeMethodName = "EnterLockScope";
internal const string LockScopeTypeName = "Scope";
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want to see if API review wants these to be public.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably want to see if API review wants these to be public.

We can certainly consider that, but we don't have to make them public "by default".

333fred
333fred reviewed Feb 13, 2024
View reviewed changes
src/Compilers/VisualBasic/Portable/VBResources.resx Outdated
<value>A value of type 'System.Threading.Lock' in SyncLock will use likely unintended monitor-based locking. Consider manually calling 'Enter' and 'Exit' methods in a Try/Finally block instead.</value>
</data>
<data name="WRN_LockTypeUnsupported_Title" xml:space="preserve">
<data name="ERR_LockTypeUnsupported" xml:space="preserve">
<value>A value of type 'System.Threading.Lock' in SyncLock will use likely unintended monitor-based locking. Consider manually calling 'Enter' and 'Exit' methods in a Try/Finally block instead.</value>
Copy link
Member

@333fred 333fred Feb 13, 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.

This error should be reworded, since it should no longer imply that Lock is supported, but a warning. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Would something like this work? "A value of type 'System.Threading.Lock' is not supported in SyncLock. Consider manually calling 'Enter' and 'Exit' methods in a Try/Finally block instead."

Copy link
Contributor

Choose a reason for hiding this comment

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

This error should be reworded, since it should no longer imply that Lock is supported, but a warning.

What wording do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Something like what @jjonescz suggested would work.

AlekseyTs
AlekseyTs approved these changes Feb 14, 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 20)

@jjonescz jjonescz merged commit f16bfb4 into dotnet:features/LockObject Feb 15, 2024
25 checks passed
@jjonescz jjonescz deleted the NativeLock-02-ControlFlowGraph branch February 15, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred Awaiting requested review from 333fred

Assignees
No one assigned
Labels
Area-Compilers New 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 @333fred @cston