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

Disallow dynamic calls on ref struct receivers #72674

Merged
merged 8 commits into from
Apr 5, 2024
Merged

Disallow dynamic calls on ref struct receivers #72674

merged 8 commits into from
Apr 5, 2024

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 22, 2024

Fixes #72606. Note that the behavior in the bug has slightly changed from the sharplab build used in the repro due to #71421.

@333fred
Disallow dynamic calls on ref struct receivers
41e69a4 
Fixes dotnet#72606. Note that the behavior in the bug has slightly changed from the sharplab build used in the repro due to dotnet#71421.
@333fred 333fred requested a review from a team as a code owner March 22, 2024 18:31
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 22, 2024
@333fred
Copy link
Member Author

333fred commented Mar 22, 2024

@dotnet/roslyn-compiler for reviews

@333fred
Copy link
Member Author

333fred commented Mar 22, 2024

@dotnet/roslyn-compiler for a second review.

@333fred
Copy link
Member Author

333fred commented Mar 25, 2024

/azp run roslyn-CI

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@333fred
Copy link
Member Author

333fred commented Mar 25, 2024

@dotnet/roslyn-compiler for reviews

@jaredpar
Copy link
Member

@AlekseyTs PTAL.

jcouv
jcouv reviewed Mar 26, 2024
View reviewed changes
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 2). Only question is on testing indexers too

@jcouv jcouv self-assigned this Mar 26, 2024
@333fred
Copy link
Member Author

333fred commented Mar 26, 2024

@jcouv addressed feedback. @jaredpar for another look at the new commit.

jcouv
jcouv previously approved these changes Mar 26, 2024
View reviewed changes
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 3)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 26, 2024 •
edited
Loading 

            expression = BindToNaturalType(expression, diagnostics);

What are the possible types of this expression? Are they dynamic and delegates only? Can we add an assert for the fact? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:473 in 0ea0eeb. [](commit_id = 0ea0eeb, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 26, 2024 •
edited
Loading 

        // HasCollectionExpressionApplicableAddMethod.bindDynamicInvocation

Should this method be adjusted too? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs:396 in 0ea0eeb. [](commit_id = 0ea0eeb, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 3)

@333fred
Copy link
Member Author

333fred commented Mar 27, 2024

Should this method be adjusted too?

Yes, but that revealed #72762. I've made the adjustment, but I think fixing the assert is outside the scope of this PR, so I opened the bug.

@AlekseyTs
Copy link
Contributor

@333fred It looks like there are some legitimate CI failures

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

@333fred
Nullable enable to address warnings, reword error message to include …
0c53f11 
…more things that it will error on.
@333fred
Merge remote-tracking branch 'upstream/main' into ref-struct-dynamic
21e1049 
* upstream/main: (416 commits)
  Semantic search (dotnet#71268)
  Make more static
  Fix MEF import of IExternalCSharpCopilotCodeAnalysisService to allow null
  Make static
  Make private
  Add comments
  Add method name to TimeInQueue telemetry (dotnet#72841)
  switch to frozen
  Simplify
  Add test
  Downstream
  Use singular helper when creating checksumsw
  Use singular helper when creating checksumsw
  Remove ability for a project to change its language
  Revert "Avoid creating result temp for is-expressions (dotnet#72273)" (dotnet#72827)
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199
  Improve generic type argument list error recovery (dotnet#69734)
  ...
@333fred
Copy link
Member Author

333fred commented Apr 3, 2024

@AlekseyTs for another review

@jcouv jcouv dismissed their stale review April 3, 2024 18:43

changed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 6)

@333fred
Merge remote-tracking branch 'upstream/main' into ref-struct-dynamic
50a7dde 
* upstream/main: (182 commits)
  inline
  inline
  move helper
  add helper
  add helper
  remove usings
  Remove special enumerator and builder
  usings
  usings
  add helper
  Remove unused usings
  Remove
  Delete accessor
  Epedited wait
  Extract helper
  Extract helper
  Seal type
  remove unused usings
  Docs
  Simplify
  ...
@333fred
Test suggestsions
0bae434
@333fred
Copy link
Member Author

333fred commented Apr 5, 2024

@AlekseyTs addressed your feedback.

@333fred
Copy link
Member Author

333fred commented Apr 5, 2024

@jcouv for another review as well.

AlekseyTs
AlekseyTs approved these changes Apr 5, 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 8), assuming CI is passing

jcouv
jcouv approved these changes Apr 5, 2024
View reviewed changes
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 8)

@333fred 333fred merged commit 7f05bbb into dotnet:main Apr 5, 2024
24 checks passed
@333fred 333fred deleted the ref-struct-dynamic branch April 5, 2024 22:02
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 5, 2024
This was referenced Apr 13, 2024
Closed
Closed
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.11 P1
Development

Successfully merging this pull request may close these issues.

Dynamic variables fail to interpolate when using a custom InterpolatedStringHandler
5 participants
@333fred @jaredpar @AlekseyTs @jcouv @dibarbet