Skip to content

Foreach ref assignment without identity conversion should give error#64879

Merged
RikkiGibson merged 9 commits intodotnet:mainfrom
ArcadeMode:foreach-ref-assignment-identity-conversion
Oct 27, 2022
Merged

Foreach ref assignment without identity conversion should give error#64879
RikkiGibson merged 9 commits intodotnet:mainfrom
ArcadeMode:foreach-ref-assignment-identity-conversion

Conversation

@ArcadeMode
Copy link
Contributor

@ArcadeMode ArcadeMode commented Oct 20, 2022

Fixes #61238

@ArcadeMode ArcadeMode requested a review from a team as a code owner October 20, 2022 21:05
@ghost ghost added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 20, 2022
@ArcadeMode
Copy link
Contributor Author

ArcadeMode commented Oct 20, 2022

The implementation reuses the existing error code CS8173 - as i mentioned in #61238 I do feel like it might be more informative for the user if the message were to read something along the lines of The enumerator must return type 'C' because it is being assigned by reference. If preferred I could introduce a new errorcode for it.

RikkiGibson
RikkiGibson reviewed Oct 20, 2022
View reviewed changes
@ArcadeMode ArcadeMode force-pushed the foreach-ref-assignment-identity-conversion branch from fbf0054 to 45e4396 Compare October 21, 2022 17:39
@runfoapp runfoapp bot mentioned this pull request Oct 21, 2022
Closed
@build-analysis build-analysis bot mentioned this pull request Oct 21, 2022
Closed
2 tasks
@jcouv
Copy link
Member

jcouv commented Oct 25, 2022

@ArcadeMode The change looks good to me. Please ping when the suggested tests are added. I'll sign-off then. Thanks

@jcouv jcouv self-assigned this Oct 25, 2022
@ArcadeMode
Copy link
Contributor Author

ArcadeMode commented Oct 25, 2022

@jcouv the mentioned test is already added, unless im overlooking something?

@jcouv
Copy link
Member

jcouv commented Oct 25, 2022

@ArcadeMode Got it. Thanks

jcouv
jcouv approved these changes Oct 25, 2022
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)

@jcouv
Copy link
Member

jcouv commented Oct 25, 2022

@RikkiGibson for second review

RikkiGibson
RikkiGibson reviewed Oct 25, 2022
View reviewed changes
RikkiGibson
RikkiGibson reviewed Oct 26, 2022
View reviewed changes
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

Very nearly done, just had a nit to pick.

@RikkiGibson
Copy link
Member

RikkiGibson commented Oct 27, 2022

Spanish test is failing. I don't think it's related to this PR.

[xUnit.net 00:01:11.47]     Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.CSharpCompletionCommandHandlerTests.TestNoBlockOnCompletionItems3(showCompletionInArgumentLists: False) [FAIL]
  Con error Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.CSharpCompletionCommandHandlerTests.TestNoBlockOnCompletionItems3(showCompletionInArgumentLists: False) [1 s]
  Mensaje de error:
   System.ObjectDisposedException : No se puede obtener acceso al objeto desechado.
Nombre del objeto: 'CompletionList`1'.
  Seguimiento de la pila:
     en Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data.CompletionList`1.CheckNotDisposed()
   en Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Data.CompletionList`1.get_IsEmpty()
   en Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Implementation.AsyncCompletionSession.GetSelectedItem(CompletionModel model)
   en Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Implementation.AsyncCompletionSession.ComputeCompletionItems(CompletionModel model)
   en Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.Implementation.AsyncCompletionSession.Microsoft.VisualStudio.Language.Intellisense.AsyncCompletion.IAsyncCompletionSession.GetComputedItems(CancellationToken token)
   en Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.TestState.CalculateItemsIfSessionExists() en /_/src/EditorFeatures/TestUtilities2/Intellisense/TestState.vb:línea 499
   en System.Threading.Tasks.Task.Execute()
--- Fin del seguimiento de la pila de la ubicación anterior donde se produjo la excepción ---
   en System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   en System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   en Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.CSharpCompletionCommandHandlerTests.VB$StateMachine_209_TestNoBlockOnCompletionItems3.MoveNext() en /_/src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb:línea 5358
--- Fin del seguimiento de la pila de la ubicación anterior donde se produjo la excepción ---
   en System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   en System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   en Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() en C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:línea 264
--- Fin del seguimiento de la pila de la ubicación anterior donde se produjo la excepción ---
   en System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   en System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   en Xunit.Sdk.ExecutionTimer.<AggregateAsync>d__4.MoveNext() en C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:línea 48
--- Fin del seguimiento de la pila de la ubicación anterior donde se produjo la excepción ---
   en System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   en System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   en Xunit.Sdk.ExceptionAggregator.<RunAsync>d__9.MoveNext() en C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:línea 90
[xUnit.net 00:01:33.25]     Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.CSharpCompletionCommandHandlerTests.TargetTypePreselectionConvertibility1 [SKIP]
Mensaje del recopilador de datos "Blame": Todas las pruebas han terminado de ejecutarse; no se generará el archivo Sequence..
  Omitidas Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.CSharpCompletionCommandHandlerTests.TargetTypePreselectionConvertibility1 [1 ms]
Results File: C:\h\w\ACB50978\w\B1F009AB\e\WorkItem_83_x64_test_results.xml

Con error! - Con error:     1, Superado:   397, Omitido:     1, Total:   399, Duración: 1 m 9 s - Microsoft.CodeAnalysis.EditorFeatures2.UnitTests.dll (net472)

@RikkiGibson RikkiGibson merged commit 9fe7909 into dotnet:main Oct 27, 2022
@ghost ghost added this to the Next milestone Oct 27, 2022
@RikkiGibson
Copy link
Member

RikkiGibson commented Oct 27, 2022

Thanks for the contribution @ArcadeMode!

@ArcadeMode ArcadeMode deleted the foreach-ref-assignment-identity-conversion branch October 27, 2022 18:09
@allisonchou allisonchou mentioned this pull request Nov 3, 2022
Closed
@allisonchou allisonchou modified the milestones: Next, 17.5 P2 Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

17.5 P2

Development

Successfully merging this pull request may close these issues.

Compiler should warn on foreach (ref T2 t2 in enumThatIsRefT1)

4 participants

@ArcadeMode @jcouv @RikkiGibson @allisonchou