Block some refactorings/fixes on top-level statements #44410
Conversation
|
Tagging @CyrusNajmabadi @sharwell @AlekseyTs |
|
I'm fine with blocking tihs. But what tracks reenabling them? I would totally expect these to work by ship time :) |
| } | ||
| </Document> | ||
| </Project> | ||
| </Workspace>", |
There was a problem hiding this comment.
surprised you used the <workspace> form here. Was that necessary?
There was a problem hiding this comment.
I didn't know it's possible to using XML approach starting with <Project> directly. Will simplify.
There was a problem hiding this comment.
Hm, on second thought, when I removed <Workspace> in test TopLevelStatement_Nested below, it started outputting XML instead of just source. I'm not sure how this works. Is it okay to leave <Workspace> here?
| </Document> | ||
| </Project> | ||
| </Workspace>", | ||
| new TestParameters(parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp8))); |
There was a problem hiding this comment.
i'm confused by the Project xml specifying the lang version and the test specifying something different.
There was a problem hiding this comment.
Yes, sorry about that.
I can remove the .WithLanguageVersion (tests would still work).
But since it's the main change I'd make to address feedback and the PR has already gone through CI, I'm tempted to leave as-is until the scenarios get properly fixed. Let me know if that's okay.
| public async Task TopLevelStatement_ArgumentInInvocation() | ||
| { | ||
| // Note: the cast should be simplified | ||
| // https://github.com/dotnet/roslyn/issues/44260 |
There was a problem hiding this comment.
this issue doesn't seem related.
There was a problem hiding this comment.
The cast gets correctly simplified for similar situation inside in a method, so the problem may be specific to top-level statement somehow.
...s/CSharp/Portable/CodeRefactorings/InlineTemporary/InlineTemporaryCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
| if (method.IsNonImplicitAndFromSource()) | ||
| { | ||
| var methodNode = root.FindNode(method.Locations[0].SourceSpan); | ||
| if (methodNode.RawKind == syntaxFacts.SyntaxKinds.GlobalStatement) |
There was a problem hiding this comment.
this is pretty ugly. tbh i'm not even sure why this works. Why does looking for the span of a method end up with the node for a GlocalStatement?
There was a problem hiding this comment.
The reason for this specific code is that later in the fixer we use this logic (root.FindNode(method.Locations[0]..., see AddParameterService.cs line 104) to find the parameter list to update. But that fails because we find a top-level statement.
The .Locations[0] seemed correct (it's the identifier/name of the local function).
But the FindNode logic has tie-breaker rule and flag getInnermostNodeForTie.
I suspect the ultimate fix might be using getInnermostNodeForTie=true, but didn't have time to explore fully.
|
|
||
| var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
| var syntaxFacts = document.GetLanguageService<ISyntaxFactsService>(); | ||
| if (localFunction && syntaxFacts.ContainsGlobalStatement(root)) |
There was a problem hiding this comment.
this is weird. so if i'm extracting to a local function from within a method in a class in a file that hsa global statements, i'm just blocked?
There was a problem hiding this comment.
At the moment yes. I agree it's weird experience :-/
MethodExtractor.CodeGenerator.GenerateAsync (line 96-100, where we handle local function scenario) and CSharpCodeGenerationService.AddStatement (line 467) is where the problem seems to be, But the logic there is complex enough that I couldn't extract/replicate a simple condition for when it won't work.
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
since this is just disabling for stability, i'm fine. but we have ot ensure that we actually fix this before shipping.
The linked issues will remain open. |
This reverts commit 3d25ced.
|
It looks like @sharwell has a solution ready for InlineTemp, so I'm reverting that portion of the PR. |
3 participants
Mitigates a few issues by disabling some fixes/refactorings in presence of top-level statements:
InlineTemporaryCodeRefactoringProvider does not work with top-level statements #44263 (InlineTemp)Relates to #43563 (test plan for top-level statements)