Skip to content

Do not show top-level-symbols inside type declarations#60131

Merged
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:topLevelLocalCompletion
Mar 12, 2022
Merged

Do not show top-level-symbols inside type declarations#60131
CyrusNajmabadi merged 1 commit intodotnet:mainfrom
CyrusNajmabadi:topLevelLocalCompletion

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 12, 2022 03:27
@ghost ghost added the Area-IDE label Mar 12, 2022
Comment on lines +11925 to +11931
class MyClass
{{
public static void F()
{{
$$
}}
}}";
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 Mar 12, 2022

Choose a reason for hiding this comment

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

Consider adding a test for partial class Program { public static void F() { $$ } }

I have a feeling that the change will break this case. They should still be filtered. Nothing should be broken :)

Comment on lines +229 to +231
// filter our top level locals if we're inside a type declaration.
if (_context.ContainingTypeDeclaration != null)
symbols = symbols.WhereAsArray(s => s.ContainingSymbol.Name != WellKnownMemberNames.TopLevelStatementsEntryPointMethodName);
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 Mar 12, 2022

Choose a reason for hiding this comment

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

Why LookupSymbols is returning the extra symbols in the first place?

I could be wrong, but to me that looks like a compiler scoping issue where the compiler works-around it in:

private static bool ReportSimpleProgramLocalReferencedOutsideOfTopLevelStatement(SimpleNameSyntax node, Symbol symbol, BindingDiagnosticBag diagnostics)
{
if (symbol.ContainingSymbol is SynthesizedSimpleProgramEntryPointSymbol)
{
if (!SyntaxFacts.IsTopLevelStatement(node.Ancestors(ascendOutOfTrivia: false).OfType<GlobalStatementSyntax>().FirstOrDefault()))
{
Error(diagnostics, ErrorCode.ERR_SimpleProgramLocalIsReferencedOutsideOfTopLevelStatement, node, node);
return true;
}
}
return false;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why LookupSymbols is returning the extra symbols in the first place?

I forget the exact reason, but it's int eh LDM notes. I think it's because if we want to add the facility for some top level symbols we can do so in a way that is sensible (e.g. imagine top level local functions in namespaces). @333fred may remember better. There's a subtle difference between not-visible, and visible-but-not-usable, and i recall there being reasons we wanted the latter.

@CyrusNajmabadi CyrusNajmabadi merged commit 35f22f2 into dotnet:main Mar 12, 2022
@ghost ghost added this to the Next milestone Mar 12, 2022
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the topLevelLocalCompletion branch May 3, 2022 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants