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

Further work on FCS API #10772

Merged
merged 24 commits into from
Dec 23, 2020
Merged

Further work on FCS API #10772

merged 24 commits into from
Dec 23, 2020

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Dec 22, 2020

This does some work (mostly to do with Range and Pos) to slowly move the FCS API towards a more intentional design surface area that we can iterate on and review and stabilise.

As background (included in a design doc in this PR), the "intended" FCS API is the parts under the namespaces

  • FSharp.Compiler.SourceCodeServices.*
  • FSharp.Compiler.Text.*
  • FSharp.Compiler.TextLayout.*

These sections are generally designed with F#/.NET design conventions (e.g. types in namespaces, not modules, no nesting of modules etc.) and we will continue to iterate to make this so.

In contrast, the public parts of the compiler directly under FSharp.Compiler.* and FSharp.AbstractIL.* are
"incidental" and not really designed for public use apart from the hook for Jet Brains Rider
(Aside: In theory all these other parts could be renamed to FSharp.Compiler.Internal though there's no need to do that right now). These internal parts tend to be implemented with the "module containing lots of stuff in one big file" approach for layers of the compiler.

The rationale here is that we eventually scrutinize everything under FSharp.Compiler.SourceCodeServices.* carefully for API design.

Because of this, I've been moving some "meant to be public" types from FSharp.Compiler.XYZ to Text. This was done for a few types in the last FCS PR. In this one we do:

  • FSharp.Compiler.Range.range --> FSharp.Compiler.Text.Range (with range abbreviation)
  • FSharp.Compiler.Range.pos --> FSharp.Compiler.Text.Pos (with pos abbreviation)

The Range module is split into Range and Pos.

This PR also

  • updates the baseline generator for FCS to only show declared members, and not inherited members, which makes the baseline tighter.

  • replaces all uses of __. by _. throughout the whole codebase.

  • adds a couple of signature files

@baronfel
Copy link
Member

For what it's worth, we use the Range and Pos module functions quite a lot in FSAC because we have to map back and forth from LSP protocol positions and ranges (which have different indexing schemes and don't have an associated file path) to positions and ranges that the FCS APIs understand. So I'd advocate for those to remain part of the FCS API and not necessarily relegated to FSharp.Compiler.

@auduchinok
Copy link
Member

I agree with @baronfel, we have some similar usages too.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

For what it's worth, we use the Range and Pos module functions quite a lot in FSAC because we have to map back and forth from LSP protocol positions and ranges (which have different indexing schemes and don't have an associated file path) to positions and ranges that the FCS APIs understand. So I'd advocate for those to remain part of the FCS API and not necessarily relegated to FSharp.Compiler.

OK This generally makes sense. If you could list out what you use that would be great

@baronfel
Copy link
Member

We use

creation functions

  • Range.mkPos
  • Range.mkRange
  • Range.mkFileIndexRange (though this one can possibly be replaced with mkRange, since the real goal here is to take a range and adjust either the start or end positions)
  • Range.unionRanges
  • Pos.fromZ

comparison functions

  • Range.posEq
  • Range.rangeContainsPos
  • Range.rangeContainsRange

identity members

  • Range.rangeStartup (for detecting if we're given a 'default'/nonsensical range from the compiler)

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

@baronfel Thanks. Yes we could put all this functionality on the types tbh (static members or instance members)

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

@baronfel Could you give me a dump on which parts of FSharp.Compiler.PrettyNaming FsAutoComplete depends on? Thanks

@auduchinok Likewise for Rider?

thanks :)

@baronfel
Copy link
Member

  • QuoteIdentifierIfNeeded
  • KeywordNames
  • TryChopPropertyName
  • IsMangledOpName
  • DemangleOperatorName
  • IsOperatorName
  • CompileOpName

are what FSAC uses at the moment.

@cartermp
Copy link
Contributor

Yeah, we use these in FSharp.Editor.dll as well. In theory this shouldn't be required, but the reality is that FCS APIs aren't nice enough to let us elide usage of this kind of stuff.

@auduchinok
Copy link
Member

auduchinok commented Dec 22, 2020

@dsyme It's this:

  • DecompileOpName
  • FsiDynamicModulePrefix
  • IsOperatorName
  • QuoteIdentifierIfNeeded
  • CompileOpName
  • IsMangledOpName
  • IsActivePatternName
  • IsInfixOperator

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

OK cool thanks all

Yeah, we use these in FSharp.Editor.dll as well. In theory this shouldn't be required, but the reality is that FCS APIs aren't nice enough to let us elide usage of this kind of stuff.

Yes I just noticed FSharp.Editor doesn't have an IVT to FSHarp.COmpiler.Private. That's great tbh, keeps us honest

I don't mind this stuff being public. I'll union the various requirements and internalise everything else, move to SourceCodeServices.

Looks like union is:

CompileOpName
DecompileOpName
DemangleOperatorName
FsiDynamicModulePrefix
FormatAndOtherOverloadsString
GetLongNameFromString
KeywordNames
IsActivePatternName
IsIdentifierFirstCharacter
IsIdentifierPartCharacter
IsInfixOperator
IsMangledOpName
IsOperatorName
IsOperatorOrBacktickedName
IsPunctuation
QuoteIdentifierIfNeeded
TryChopPropertyName

@cartermp
Copy link
Contributor

Yes I just noticed FSharp.Editor doesn't have an IVT to FSHarp.COmpiler.Private.

Yes, this was intentional :) - I had to expose a few things to make that possible, but it's the right call overall.

We should have a think about better APIs for things like going to definition or finding a declaration list. The APIs for that are too low-level/difficult to work with and require every editor to access little utility functions like this and/or build their own to get access to other data that these APIs require.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

OK I've progressed this so the full surface area is now

  • FSharp.Compiler.SourceCodeServices.* (analysis, compilation, tooling, lexing)
  • FSharp.Compiler.Interactive.Shell.* (scripting support)
  • FSharp.Compiler.AbstractIL.* (for ILAssemblyReader hook for Rider)
  • FSharp.Compiler.SyntaxTree.* (direct access to full untyped tree)

There is essentially nothing public directly in FSharp.Compiler.*

@TIHan I moved ISourceText from FSharp.Compiler.Text into FSharp.Compiler.SourceCodeServices to unify the actual intended public surface area as much as possible for now. I know this might seem like a slight step backward, but my overall aim here is to get to the point were we can start an intentional community design process of looking at the remaining public contents and deciding what we want to do with renaming, organising into namespaces, deprecating etc. I'll start a separate issue on this, and we can compare with Roslyn too.

@cartermp
Copy link
Contributor

@dsyme we should probably get some kind of documentation into an RFC as well, and let that serve as a process for using RFCs to drive new API work in FCS. For small additions it's probably not an issue, but eventually we'll want to do something about the fact that we expose our tree and that any additions to it will break all consumers. And we definitely want to make additions to it over time.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

@cartermp Yup. Have started a discussion thread here: #10786 I think it's ok to do this one via discussion in this repo, no formal RFC needed

@dsyme
Copy link
Contributor Author

dsyme commented Dec 23, 2020

@TIHan I moved ISourceText from FSharp.Compiler.Text into FSharp.Compiler.SourceCodeServices ...

I thought this over and I'm undoing this. FSharp.Compiler.Text is a perfectly good namespace and we should really move Range and Pos and maybe some other basic text related things into there

@dsyme
Copy link
Contributor Author

dsyme commented Dec 23, 2020

This is ready now

@dsyme
Copy link
Contributor Author

dsyme commented Dec 23, 2020

@cartermp @vzarytovskii @KevinRansom @TIHan approval requested to get this merged and integrated with other branch work. I know there are a lot of files touched but the vast majority are just a couple of lines of 'open' decl changes or entirely cosmetic.

@KevinRansom KevinRansom merged commit b23b2a0 into dotnet:main Dec 23, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Dec 23, 2020

Thanks @vzarytovskii @KevinRansom

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* trim API surface area

* move Range and Pos types to FSharp.Compiler

* undo uppercae range/pos

* move Range and Pos types to FSHarp.Compiler.SourceCodeServices since they are part of the FCS API

* fix baselines

* release notes

* FIX BUILD

* fix test

* fix test

* fix test

* fix test

* fix build

* fix build

* fix build

* PrettyNaming moved to SourceCodeServices

* ISourceText moved to SourceCodeServices

* update docs

* update docs

* fix build

* fix build

* move Range, Pos to FSharp.Compiler.Text

* fix test

* fix build

Co-authored-by: Don Syme <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* trim API surface area

* move Range and Pos types to FSharp.Compiler

* undo uppercae range/pos

* move Range and Pos types to FSHarp.Compiler.SourceCodeServices since they are part of the FCS API

* fix baselines

* release notes

* FIX BUILD

* fix test

* fix test

* fix test

* fix test

* fix build

* fix build

* fix build

* PrettyNaming moved to SourceCodeServices

* ISourceText moved to SourceCodeServices

* update docs

* update docs

* fix build

* fix build

* move Range, Pos to FSharp.Compiler.Text

* fix test

* fix build

Co-authored-by: Don Syme <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* trim API surface area

* move Range and Pos types to FSharp.Compiler

* undo uppercae range/pos

* move Range and Pos types to FSHarp.Compiler.SourceCodeServices since they are part of the FCS API

* fix baselines

* release notes

* FIX BUILD

* fix test

* fix test

* fix test

* fix test

* fix build

* fix build

* fix build

* PrettyNaming moved to SourceCodeServices

* ISourceText moved to SourceCodeServices

* update docs

* update docs

* fix build

* fix build

* move Range, Pos to FSharp.Compiler.Text

* fix test

* fix build

Co-authored-by: Don Syme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants