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

SE - Nullable: Add UT for conversions #6857

Merged
merged 5 commits into from
Mar 8, 2023
Merged

SE - Nullable: Add UT for conversions #6857

merged 5 commits into from
Mar 8, 2023

Conversation

pavel-mikula-sonarsource
Copy link
Contributor

Part of #6812

Add UTs for implicit and explicit conversions to be sure they work as expected.

martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

The PreProcessCheck seems unnecessary.

...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs Outdated Show resolved Hide resolved
...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs Outdated Show resolved Hide resolved
...ests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/RoslynSymbolicExecutionTest.Nullable.cs Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an IsUserDefined property. I would propose using that to check for user-defined conversions.

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/OperationProcessors/Conversion.cs Outdated
@@ -26,7 +26,7 @@ internal sealed class Conversion : SimpleProcessor<IConversionOperationWrapper>
operation.ToConversion();

protected override ProgramState Process(SymbolicContext context, IConversionOperationWrapper conversion) =>
context.State[conversion.Operand] is { } value
context.State[conversion.Operand] is { } value && conversion.OperatorMethod is null
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the "is user defined conversion" check? If so, it would be better to check IsUserDefined. It is accessible via the Conversion property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is user defined when the symbol is not null, that's what I've observed when I compared few cases and also here
https://github.com/dotnet/roslyn/blob/aff2a21bbf6726f0ecab4660568e068aa923ef55/src/Compilers/Core/Portable/Operations/CommonConversion.cs#L72-L81

So those two conditions are functionally equivalent. IsUserDefined is definitely more readable, unfortunately, CommonConversion is not available in the ShimLayer and would require heavy scaffolding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. Can we add a small comment like // is a built-in conversion?

I do remember that there were some surprises with built-in conversions for decimal as it reported a conversion method back when I worked on the operator intellisense. It might be worth adding test cases for decimal and System.Half. If there is something wrong we can whitelist these symbols in a later PR.

Base automatically changed from Pavel/SE/04-NewArg to feature/SE March 8, 2023 13:40
@pavel-mikula-sonarsource pavel-mikula-sonarsource marked this pull request as ready for review March 8, 2023 13:42
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

A System.Half and decimal test cases might reveal a surprise.

@pavel-mikula-sonarsource
Copy link
Contributor Author

A System.Half and decimal test cases might reveal a surprise.

System.Half is surprising, it has IsUserDefined = true.
decimal is fine.

Tests added

martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. You may want to add a code comment to conversion.OperatorMethod is null like // built-in conversions only

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit d75929a into feature/SE Mar 8, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Pavel/SE/05-Conversions branch March 8, 2023 16:49
pavel-mikula-sonarsource added a commit that referenced this pull request Mar 14, 2023
@pavel-mikula-sonarsource
SE - Nullable: Add UT for conversions (#6857)
c7a4a68
pavel-mikula-sonarsource added a commit that referenced this pull request Mar 20, 2023
@pavel-mikula-sonarsource
SE - Nullable: Add UT for conversions (#6857)
6e9863b
pavel-mikula-sonarsource added a commit that referenced this pull request Mar 28, 2023
@pavel-mikula-sonarsource
SE - Nullable: Add UT for conversions (#6857)
5eec5b6
pavel-mikula-sonarsource added a commit that referenced this pull request Mar 31, 2023
@pavel-mikula-sonarsource
SE - Nullable: Add UT for conversions (#6857)
b3db7b0
pavel-mikula-sonarsource added a commit that referenced this pull request Apr 4, 2023
@pavel-mikula-sonarsource
SE - Nullable: Add UT for conversions (#6857)
8e8949f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-strecker-sonarsource martin-strecker-sonarsource martin-strecker-sonarsource approved these changes

Assignees

@pavel-mikula-sonarsource pavel-mikula-sonarsource

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pavel-mikula-sonarsource @martin-strecker-sonarsource