Skip to content

API for exposing imports (including global imports) from the semantic model#59533

Merged
CyrusNajmabadi merged 67 commits intodotnet:mainfrom
CyrusNajmabadi:compilationImports
Apr 28, 2022
Merged

API for exposing imports (including global imports) from the semantic model#59533
CyrusNajmabadi merged 67 commits intodotnet:mainfrom
CyrusNajmabadi:compilationImports

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Feb 14, 2022

Adding a new API to provide the set of imported symbols at a particular location in a code file.

The API shape is:

public class SemanticModel
{
+ public ImmutableArray<IImportScope> GetImportScopes(int position, CancellationToken cancellationToken = default);
}

+    /// <summary>
+    /// Represents a chain of symbols that are imported to a particular position in a source file.  Symbols may be
+    /// imported, but may not necessarily be available at that location (for example, an alias symbol hidden by another
+    /// symbol).  There is no guarantee that the same chain will be returned from successive calls to <see
+    /// cref="SemanticModel.GetImportScopes"/>.  Each import has a reference to the location the import directive was
+    /// declared at.  For the <see cref="IAliasSymbol"/> import, the location can be found using either <see
+    /// cref="ISymbol.Locations"/> or <see cref="ISymbol.DeclaringSyntaxReferences"/> on the <see cref="IAliasSymbol"/>
+    /// itself.  For <see cref="Imports"/> or <see cref="XmlNamespaces"/> the location is found through <see
+    /// cref="ImportedNamespaceOrType.DeclaringSyntaxReference"/> or <see
+    /// cref="ImportedXmlNamespace.DeclaringSyntaxReference"/> respectively.
+    /// </summary>
+    /// <remarks>
+    /// <list type="bullet">
+    /// Scopes returned will always have at least one non-empty property value in them.
+    /// <item>
+    /// In C# there will be an <see cref="IImportScope"/> for every containing namespace-declarations that include any
+    /// import directives.  There will also be an <see cref="IImportScope"/> for the containing compilation-unit if it
+    /// includes any import directives or if there are global import directives pulled in from other files.
+    /// </item>
+    /// <item>
+    /// In Visual Basic there will only be at most two <see cref="IImportScope"/>s returned for any position.  There
+    /// will be a scope for the containing compilation unit if it includes any import directives.  There can also be a
+    /// scope representing any imports specified at the project level.
+    /// </item>
+    /// <item>
+    /// Elements of any property have no defined order.  Even if they represent items from a single document, they are
+    /// not guaranteed to be returned in any specific file-oriented order.
+    /// </item>
+    /// </list>
+    /// </remarks>
+    public interface IImportScope
+    {
+        /// <summary>
+        /// Aliases defined at this level of the chain.  This corresponds to <c>using X = TypeOrNamespace;</c> in C# or
+        /// <c>Imports X = TypeOrNamespace</c> in Visual Basic.
+        /// </summary>
+        ImmutableArray<IAliasSymbol> Aliases { get; }
+
+        /// <summary>
+        /// Extern aliases defined at this level of the chain.  This corresponds to <c>extern alias X;</c> in C#.  It
+        /// will be empty in Visual Basic.
+        /// </summary>
+        ImmutableArray<IAliasSymbol> ExternAliases { get; }
+
+        /// <summary>
+        /// Types or namespaces imported at this level of the chain.  This corresponds to <c>using Namespace;</c> or
+        /// <c>using static Type;</c> in C#, or <c>Imports TypeOrNamespace</c> in Visual Basic.
+        /// </summary>
+        ImmutableArray<ImportedNamespaceOrType> Imports { get; }
+
+        /// <summary>
+        /// Xml namespaces imported at this level of the chain.  This corresponds to <c>Imports &lt;xmlns:prefix =
+        /// "name"&gt;</c> in Visual Basic.  It will be empty in C#.
+        /// </summary>
+        ImmutableArray<ImportedXmlNamespace> XmlNamespaces { get; }
+    }
+
+    /// <summary>
+    /// Represents an <see cref="INamespaceOrTypeSymbol"/> that has been imported, and the location the import was
+    /// declared at.  This corresponds to <c>using Namespace;</c> or <c>using static Type;</c> in C#, or <c>Imports
+    /// TypeOrNamespace</c> in Visual Basic.
+    /// </summary>
+    public readonly struct ImportedNamespaceOrType
+    {
+        public INamespaceOrTypeSymbol NamespaceOrType { get; }
+        public SyntaxReference? DeclaringSyntaxReference { get; }
+
+        internal ImportedNamespaceOrType(INamespaceOrTypeSymbol namespaceOrType, SyntaxReference? declaringSyntaxReference)
+        {
+            NamespaceOrType = namespaceOrType;
+            DeclaringSyntaxReference = declaringSyntaxReference;
+        }
+    }
+
+    /// <summary>
+    /// Represents an imported xml namespace name. This corresponds to <c>Imports &lt;xmlns:prefix = "name"&gt;</c> in
+    /// Visual Basic.  It does not exist for C#.
+    /// </summary>
+    public readonly struct ImportedXmlNamespace
+    {
+        public string XmlNamespace { get; }
+        public SyntaxReference? DeclaringSyntaxReference { get; }
+
+        internal ImportedXmlNamespace(string xmlNamespace, SyntaxReference? declaringSyntaxReference)
+        {
+            XmlNamespace = xmlNamespace;
+            DeclaringSyntaxReference = declaringSyntaxReference;
+        }
+    }

TODO:

  • Agreed upon API shape
  • C# Tests
  • VB tests

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 14, 2022 17:51
@ghost ghost added the Area-Compilers label Feb 14, 2022
@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft February 14, 2022 17:51
/// <summary>
/// Represents a chain of symbols that are imported to a particular position in a source file. Symbols may be
/// imported, but may not necessarily be available at that location (for example, an alias symbol hidden by another
/// symbol).
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.

note: i hope we keep the last part. or, if not, we have some really easy way of determining which import symbols should be stripped out because they're not available :)

{
[DebuggerDisplay("{GetDebuggerDisplay(),nq}")]
internal sealed class ImportChain : Cci.IImportScope
internal sealed class ImportChain : Cci.IImportScope, IImportChain
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.

that c# already had this 'ImportChain' type was a lot of the inspiration of this this approach. It has already stitched together the entire chain, so why not just use it?

''' </summary>
Friend Class ImportAliasesBinder
Inherits Binder
Implements IImportChain
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.

in VB things are a bit different. the binder is itself the link in the chain. So two of the binders then expose the relevant information. Specifically ImportAliasesBinder and TypesOfImportedNamespacesMembersBinder. I got thes two binders from CreateBinderForSourceFile which showed me how you guys stitch up that chain yourself.

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.

not sure how acceptable/appropriate this approach is for you two.

@333fred
Copy link
Copy Markdown
Member

333fred commented Feb 14, 2022

API shape itself seems perfectly reasonable to me, and was about what I was expecting to see.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@333fred Cool! Any concern about the impl side? I'm not an expert on binders and i wasn't sure if this was a reasonable way to go.

@333fred
Copy link
Copy Markdown
Member

333fred commented Feb 14, 2022

@333fred Cool! Any concern about the impl side? I'm not an expert on binders and i wasn't sure if this was a reasonable way to go.

Exposing binders directly like this definitely gives me a bit of pause. I'd want to think on it a bit.

@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 2)

Copy link
Copy Markdown
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 62). Please make sure to squash (after second sign-off). Much appreciate

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@AlekseyTs for eyes. thanks :)

End Sub

Public Function GetImportChainData() As ImmutableArray(Of IAliasSymbol)
Return ImmutableArray(Of IAliasSymbol).CastUp(_importedAliases.SelectAsArray(Function(kvp) kvp.Value.Alias))
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Apr 22, 2022

Choose a reason for hiding this comment

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

_importedAliases.SelectAsArray

The order doesn't look deterministic #Closed

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.

Currently, that's intentional. I've explicitly stated in the docs that order is not guaranteed. Do you think that's sufficient/ or would you prefer we stated some guarantee here and then coded that in?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Currently, that's intentional. I've explicitly stated in the docs that order is not guaranteed. Do you think that's sufficient/ or would you prefer we stated some guarantee here and then coded that in?

In general we are trying to avoid behavior like that in a public API. Even if we do not document any particular order, we are trying to avoid "randomization" for the same scenario, which is likely to happen in this case. I think we don't need to block this PR, but let's bring this issue for discussion at the API review meeting.

@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with review pass (commit 62)

Copy link
Copy Markdown
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 62) with a follow-up on the ordering issue.

@jcouv jcouv self-requested a review April 27, 2022 17:04
@jcouv jcouv self-assigned this Apr 27, 2022
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@AlekseyTs @jcouv if you'd like to take a look after the latest changes. This applies the outcome of the api review that it's ok that things not be ordered. Note: the docs still say that things are not ordered. Would you like me to remove those docs and just provide no statement on it? Or should we leave the docs in? Thanks!

@jcouv jcouv requested a review from AlekseyTs April 28, 2022 16:52
Copy link
Copy Markdown
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 67)

Copy link
Copy Markdown
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 67)

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

Thanks all! Set to squash and merge.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants