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.

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

Public API for "file-local types" #62254

Closed
RikkiGibson opened this issue Jun 29, 2022 · 2 comments
Closed

Public API for "file-local types" #62254

RikkiGibson opened this issue Jun 29, 2022 · 2 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - File-Local Types File-local types (file types) Feature Request
Milestone

Comments

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Jun 29, 2022

Background and Motivation

The file-local types feature (#60819) introduces public APIs.
[update:] API is named IsFileLocal

Proposed API

namespace Microsoft.CodeAnalysis
{
     public interface INamedTypeSymbol
     {
+        /// <summary>
+        /// Indicates that this type is only visible in the file it was declared in.
+        /// </summary>
+        bool IsFile { get; }
     }
}
namespace Microsoft.CodeAnalysis.CSharp
{
    public enum SyntaxKind
    {
        // ...

        /// <summary>Represents <see langword="required"/>.</summary>
        RequiredKeyword = 8447,

+        /// <summary>Represents <see langword="file"/>.</summary>
+        FileKeyword = 8448,
    }
}

Usage Examples

INamedTypeSymbol.IsFile is needed to implement SymbolKey for file types.

private static class NamedTypeSymbolKey
{
public static void Create(INamedTypeSymbol symbol, SymbolKeyWriter visitor)
{
visitor.WriteSymbolKey(symbol.ContainingSymbol);
visitor.WriteString(symbol.Name);
visitor.WriteInteger(symbol.Arity);
visitor.WriteString(symbol.IsFile
? symbol.DeclaringSyntaxReferences[0].SyntaxTree.FilePath
: null);
visitor.WriteBoolean(symbol.IsUnboundGenericType);

private static void Resolve(
PooledArrayBuilder<INamedTypeSymbol> result,
INamespaceOrTypeSymbol container,
string name,
int arity,
string? filePath,
bool isUnboundGenericType,
ITypeSymbol[] typeArguments)
{
foreach (var type in container.GetTypeMembers(name, arity))
{
// if this is a 'file' type, then only resolve to a file-type from this same file
if (filePath != null)
{
if (!type.IsFile ||
type.DeclaringSyntaxReferences.IsEmpty ||
type.DeclaringSyntaxReferences[0].SyntaxTree.FilePath != filePath)
{
continue;
}
}

The IDE may also have other places where they want to handle a file type specially. For example, when it is displayed in the navbar or member list.

Whenever the IDE gets the members of a namespace symbol, they may need to filter out file types which aren't available in the current file, for example. LookupSymbols does this automatically when it is given a INamespaceOrTypeSymbol? container but INamespaceSymbol.GetMembers() won't be able to do this automatically.

Alternative Designs

We're not proposing any alternative designs here. We think it's not viable to proceed with IDE tooling for the feature without the INamedTypeSymbol.IsFile API. It might be possible to go to source for some cases, since file types is a source-only concept, but it is expensive and would break layering for symbol-oriented components to do so. This is something the IDE team strongly wants to avoid.

Earlier on we considered having an API SyntaxTree? INamedTypeSymbol.AssociatedSyntaxTree, where a non-null return value means the symbol is a file type, and a null return value means it is not a file type. However, it was felt that exposing just bool INamedTypeSymbol.IsFile is simpler here. Users who want to actually know which file the symbol is declared in can check DeclaringSyntaxReferences.

Risks

Not aware of any risks from the current design.

SymbolDisplay

We've added internal symbol display options to make it easier to distinguish file types declared in different files with the same name.

// FilePath: path/to/MyFile.cs
// displays as 'C@MyFile' with ToTestDisplayString()
file class C { }

// FilePath: path/to/OtherFile.cs
// displays as 'C@OtherFile' with ToTestDisplayString()
file class C { }

This raises a few questions:

  1. Should the specific way this is displayed be changed? i.e. is there something better to denote the "file name" suffix than an @ symbol?
  2. Should we make this symbol display option public?
namespace Microsoft.CodeAnalysis
{
    internal enum SymbolDisplayCompilerInternalOptions
    {
        // ...

        /// <summary>
        /// Separate out nested types from containing types using <c>+</c> instead of <c>.</c> (dot).
        /// </summary>
        UsePlusForNestedTypes = 1 << 8,

+        /// <summary>
+        /// Display `MyType@File` instead of `MyType`.
+        /// </summary>
+        IncludeContainingFileForFileTypes = 1 << 9,
    }
@RikkiGibson RikkiGibson added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Jun 29, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 29, 2022
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 29, 2022
@jcouv jcouv added this to the 17.4 milestone Jun 29, 2022
@RikkiGibson RikkiGibson added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jun 29, 2022
@RikkiGibson RikkiGibson changed the title Public API for "file types" Public API for "file-local types" Jul 7, 2022
@333fred
Copy link
Member

333fred commented Jul 7, 2022

API Reviews

  • IsFileLocal instead of IsFile
  • Could we implement metadata support?
    • Technically, yes, it could
    • However, local to which file? That data isn't preserved
  • Expand a bit on the doc comment: mention that it's only true for source, clarify the implementation details.
  • No public use case at the moment, we can look at it again when we have a use case

Conclusion

Approved, with IsFileLocal instead of IsFile. We will not expose a symbol display option for now.

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 7, 2022
@jaredpar jaredpar added the Feature - File-Local Types File-local types (file types) label Jul 8, 2022
@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Jul 22, 2022

We've implemented the APIs that were approved, so will close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - File-Local Types File-local types (file types) Feature Request
Projects
None yet
Development

No branches or pull requests

5 participants