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

Only Left and returnValue gets checked for dynamic in signature #69214

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

gero3
Copy link
Contributor

@gero3 gero3 commented Jul 25, 2023

In CSharpcompilation there was a signature check for dynamic Types but only left and return were checked. I was running my roslyn analyser to check for identical subexpressions over this repository to get some edgecases that I forgot about. This came out as response

In CSharpcompilation there was a signature check for dynamic Types but only left and return were checked
@gero3 gero3 requested a review from a team as a code owner July 25, 2023 20:03
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 25, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 25, 2023
@gero3
Copy link
Contributor Author

gero3 commented Jul 25, 2023

This should resolve #69215.

@@ -4127,7 +4127,7 @@ void validateSignature()
// Dynamic built-in operators allow virtually all operations with all types. So we do no further checking here.
if (csharpReturnType.TypeKind is TypeKind.Dynamic ||
csharpLeftType.TypeKind is TypeKind.Dynamic ||
csharpReturnType.TypeKind is TypeKind.Dynamic)
csharpRightType.TypeKind is TypeKind.Dynamic)
Copy link
Member

Choose a reason for hiding this comment

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

definitely looks like a good bug you've caught. :)

would need a test to go along with this. Thsi also might be a breaking change in some way. Compiler will have to look into that and see if this needs to be doc'ed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it will functionally change something because I can't seem to figure out how to have the right dynamic type without a return type that is also dynamic.

@gero3
Copy link
Contributor Author

gero3 commented Jul 26, 2023

As well as I understand the function of the code, the function validateSignature should reject all invalid signatures for creating
a SynthesizedIntrinsicOperatorSymbol. So It should throw when the type of result is nondynamic while either left or right is dynamic. This is based on the debug assert in the constructor of SynthesizedIntrinsicOperatorSymbol found on Line 41.

I don't know if this needs extra checks due to only being Debug.Assert.

Otherwise I would change the code to the following:

               if (csharpReturnType.TypeKind is TypeKind.Dynamic && csharpLeftType.TypeKind is TypeKind.Dynamic ||
                   csharpReturnType.TypeKind is TypeKind.Dynamic && csharpRightType.TypeKind is TypeKind.Dynamic)
               {
                   return;
               }

I could also add tests to check if it throws when the returntype is non dynamic while left and/or right are dynamic.

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 1)

@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-compiler For the second review

@jaredpar jaredpar added this to the 17.8 milestone Jul 28, 2023
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 1)

@jcouv jcouv self-assigned this Aug 4, 2023
@jcouv jcouv merged commit 0602129 into dotnet:main Aug 4, 2023
@ghost ghost modified the milestones: 17.8, Next Aug 4, 2023
@jcouv
Copy link
Member

jcouv commented Aug 4, 2023

Thanks for the contribution @gero3 !

@gero3
Copy link
Contributor Author

gero3 commented Aug 4, 2023

No problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. 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.

6 participants