Skip to content

Infer delegate types with -langversion:preview only#53241

Merged
cston merged 3 commits intodotnet:features/compilerfrom
cston:infer-delegate-preview-only
May 12, 2021
Merged

Infer delegate types with -langversion:preview only#53241
cston merged 3 commits intodotnet:features/compilerfrom
cston:infer-delegate-preview-only

Conversation

@cston
Copy link
Contributor

@cston cston commented May 6, 2021 •
edited by jcouv
Loading

See #52448.
Test plan: #52192

@cston cston requested a review from a team as a code owner May 6, 2021 22:47
@ghost ghost added the Area-Compilers label May 6, 2021
@jcouv jcouv added this to the C# 10 milestone May 6, 2021
@jcouv jcouv mentioned this pull request May 7, 2021
Closed
@jcouv
Copy link
Member

jcouv commented May 7, 2021 •
edited by cston
Loading 

        if (delegateOrFuncPtrType.SpecialType != SpecialType.System_Delegate)

Do we need to adjust this one too?

In reply to: 834049365

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1189 in 366686a. [](commit_id = 366686a, deletion_comment = False)

jcouv
jcouv approved these changes May 7, 2021
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) with one question

@jcouv jcouv self-assigned this May 7, 2021
@cston
Copy link
Contributor Author

cston commented May 7, 2021 •
edited
Loading 

        if (delegateOrFuncPtrType.SpecialType != SpecialType.System_Delegate)

Do we need to adjust this one too?

I don't believe so, since this method should only be called in cases where Conversions.GetMethodGroupDelegateConversion() classified the conversion as a method group conversion, and GetMethodGroupDelegateConversion() checks that the feature is enabled.

In reply to: 834049365

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:1189 in 366686a. [](commit_id = 366686a, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 7, 2021

Infer delegate types with -langversion:preview only

Is this a permanent or a temporary change? It feels like it violates the general principle that language version doesn't affect result of semantic analysis, apart from diagnostics.

@cston
Copy link
Contributor Author

cston commented May 7, 2021 •
edited
Loading

Is this a permanent or a temporary change? It feels like it violates the general principle that language version doesn't affect result of semantic analysis, apart from diagnostics.

This change is temporary and intended for early previews only.

@jcouv
Copy link
Member

jcouv commented May 7, 2021

This change is temporary and intended for early previews only.

Btw, I added a note to test plan as a reminder to revert this change (assuming it gets merged).

@cston
Copy link
Contributor Author

cston commented May 11, 2021

@dotnet/roslyn-compiler for a second review of a temporary change for early previews only.

@cston cston requested a review from a team May 12, 2021 16:20
AlekseyTs
AlekseyTs approved these changes May 12, 2021
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 3)

@cston cston merged commit 82ce937 into dotnet:features/compiler May 12, 2021
@jcouv jcouv mentioned this pull request May 12, 2021
Merged
cston added a commit to cston/roslyn that referenced this pull request May 17, 2021
@cston
Revert "Infer delegate types with -langversion:preview only (dotnet#5…
8b4f4b5 
…3241)"

This reverts commit 82ce937.
@cston cston mentioned this pull request May 17, 2021
Merged
@cston cston modified the milestones: C# 10, 16.11 May 17, 2021
cston added a commit that referenced this pull request May 18, 2021
@cston
Revert "Infer delegate types with -langversion:preview only (#53241)" (…
7d461ed 
…#53466)

This reverts commit 82ce937.
333fred added a commit to 333fred/roslyn that referenced this pull request May 20, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into editorformattingser…
e1f3b59 
…vice-featureslayer

* upstream/main: (857 commits)
  Update contrib documentation (dotnet#53504)
  SImplify
  Fix out of bound crash in lsp navto.
  Revert changes to TypeScriptWaitContext wrappers
  Switch to ROSLYN_TEST_CI for CI detection
  SImplify
  Simplify LoggerTestChannel using BlockingCollection
  Only require telemetry validation in CI
  Fix out of bound crash in lsp navto.
  Fix locked comment
  Update Compiler Test Plan.md (dotnet#53420)
  Adjust doc comment for NullableWalker.VisitConversion (dotnet#53429)
  Revert "Infer delegate types with -langversion:preview only (dotnet#53241)" (dotnet#53466)
  Fix syntax normalizer to add space around before colon in constructor initializer (dotnet#53326)
  Remove unnecessary property (dotnet#53406)
  EnC - Tell the debugger about updated type def tokens (dotnet#53217)
  Revert an error
  Update PublishData.json
  Keep trailing trivia so single line if statements don't get badly formatted (dotnet#53414)
  Fix dead test code (dotnet#53416)
  ...
cston added a commit to cston/roslyn that referenced this pull request Jul 29, 2021
@cston
Revert "Revert "Infer delegate types with -langversion:preview only (d…
9894189 
…otnet#53241)" (dotnet#53466)"

This reverts commit 7d461ed.
@cston cston mentioned this pull request Jul 29, 2021
Merged
cston added a commit that referenced this pull request Jul 29, 2021
@cston
Infer delegate types with -langversion:10 (and above) only (#55232)
a429a88 
Revert "Revert "Infer delegate types with -langversion:preview only (#53241)" (#53466)"

This reverts commit 7d461ed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Lambda Improvements

Projects

None yet

Milestone

16.11

Development

Successfully merging this pull request may close these issues.

3 participants

@cston @jcouv @AlekseyTs