Skip to content

Commit

Permalink
Merge pull request dotnet#45211 from 333fred/api-notes
Browse files Browse the repository at this point in the history
Add API review notes for d16.7
  • Loading branch information
333fred authored Jun 23, 2020
2 parents 430056a + a8d8245 commit 6230bcd
Showing 1 changed file with 61 additions and 0 deletions.
61 changes: 61 additions & 0 deletions docs/compilers/API Notes/6-11-20.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
### `BaseTypeSyntax` Design

In master, we currently have added an optional argument list to `SimpleBaseTypeSyntax` for record primary constructors. However, there is an argument to be made that we should add a new subtype of `BaseTypeSyntax`, something like `PrimaryConstructorTypeSyntax`, to represent these new base type clauses. This was an explicit extension point added in the original design for this very purpose. For either of these designs, GetSymbolInfo will behave consistently: if the `BaseTypeSyntax` has an argument list (either because it's a separate type or because it's a `PrimaryConstructorTypeSyntax`), it will return the constructor symbol or candidates as appropriate. If there are no arguments, then it will return nothing.

Pros for keeping the optional argument list on `SimpleBaseTypeSyntax`:
* The primary constructor arguments feel like an extension to existing nodes, not a totally new node type
* Existing analyzers/fixers might pick things up right without much modification

Pros for extending `BaseTypeSyntax`:
* This extension point was put in explicitly for this
* Existing code might be less likely to erroneously use it
* There was some feeling that it would serve as a more clear delineation for future additions to this syntax (if such C# features ever come to pass)

#### Conclusion

We lean towards extending `BaseTypeSyntax` in the next preview.

### `CompilationOutputFilePaths`

The plural when this struct only has one path in it currently felt wrong. Some options for renaming:

* `CompilationOutputFileInfo`
* `CompilationOutputInfo`
* `CompilationOutputFiles`

We leave it up to whoever does the rename to choose among these.

### `SolutionInfo.Create`

Add `EditorBrowsable(Never)` to the backcompat overload.

### `SymbolKind.FunctionPointer`

Suffix with `Type` for consistency with the other kinds that are also `TypeKind`s.

### `SyntaxFactory.ParseType`

The new overload should use `ParseOptions` instead of the specific C#/VB type in both factories.
Most of the Parse overloads that take a `ParseOptions` here use the non-specific one, and it can be very convenient for IDE APIs here.
While it is marginally less type-safe, the ergonomic and consistency benefits are a deciding factor.

### `WithExpressionSyntax.Receiver`

This should be renamed to `Expression` for consistency with other `SyntaxNode`s.

### `SymbolFinder.Find(Derived(Classes|Interfaces)|Interfaces)Async`

We should reconsider adding multiple overloads, and instead remove the default parameters from the existing overload and introduce a new overload with `transitive` set to `true` to match current behavior.
This is our standard modus operandi for these scenarios.

### `INegatedPatternOperation.NegatedPattern`

Rename to just `Pattern`.

### `IWithOperation.Value`

Rename to `Operand`.

### `Renamer.RenameDocumentAsync`

Make this and the related structs internal unless we have a partner team ready to consume the API in 16.7. We can make the API public again when someone is ready to consume and validate the API shape.

0 comments on commit 6230bcd

Please sign in to comment.