Skip to content

[WIP] use Layout as a source data of classification related tasks #2070

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

Merged
merged 17 commits into from
Jan 3, 2017

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Dec 21, 2016

Early WIP
F# compiler uses Layout structure for pretty printing but ultimately outputs result as a string. This become a culprit in classification related scenarios since in order to colorize everything nicely we have to effectively recover original Layout. This PR introduces TaggedText annotations for text chunks in Layout. Later annotated text chunks can easily be converted from the Layout to Roslyn TaggedText.

Remaining items:

  • - add glyphs to quick info. Possible approach is proposed #1 but I think we can do this without doing extra request for symbol.
  • - make sure that formatting in final result matches current version (though differences might be ok if they improve the experience)
  • - fix existing tests
  • - clean-up API surface - currently PR changes shapes/signatures of existing types and methods which will be a breaking change for anyone who consumed previous version of the compiler. Existing API that deal with string should be reverted back and new API that use Layout should be added instead of replacing existing ones.

per @vasily-kirichenko' comment:

  • - bugfixes

Quick info:
image

Signature help:
image

Completion details:
image

@dungpa
Copy link
Contributor

dungpa commented Dec 21, 2016

Woohoo! I've been waiting for this PR for so long.

@vasily-kirichenko
Copy link
Contributor

It's just fantastic 🥇 🥇 🥇

@vasily-kirichenko
Copy link
Contributor

As you have not mentioned any bugs in the to do list, I feel free to report first ones :)

Here type should be highlighted as keyword:

image

The namespace should not be highlighted, bool keyword is not highlighted, ThreadPool type is not highlighted:

image

@saul
Copy link
Contributor

saul commented Dec 21, 2016

This look absolutely fantastic!

Another issue is that the indentation seems to be lost:
sketch 13

@vasily-kirichenko
Copy link
Contributor

@vladima do I understand right that this color information can be used for normal semantic code highlighting in the editor?

@vladima
Copy link
Contributor Author

vladima commented Dec 22, 2016

I'm not sure that I understand the question: compiler uses layouts to pretty print various kinds of messages but it is not used to format the source text itself. For classification in the editor one should use IEditorClassificationService which I think F# language service already does

@saul
Copy link
Contributor

saul commented Dec 25, 2016

Another bug - type parameters include the apostrophe prefix in the colouring. The editor doesn't colour the apostrophe when doing syntax highlighting - I'm not sure which is the preferred option here.

Another positive is that FSI's output can now be colourised too:
image

@cloudRoutine
Copy link
Contributor

@saul including the apostrophe is correct, the current highlighting in the editor is wrong

@vladima
Copy link
Contributor Author

vladima commented Dec 27, 2016

per F# spec:

typar :=
_ -- anonymous variable type
'ident -- type variable
^ident -- static head-type type variable

so I read it as ' is considered part of the name

@vladima
Copy link
Contributor Author

vladima commented Dec 27, 2016

status update: fixed indentation and classification of types inside xml doc comments
image

image

@saul
Copy link
Contributor

saul commented Dec 30, 2016

Another small issue - I've noticed that "Full name: XXX" seems to be indented by a couple of spaces. This is not the case on master without your PR, so looks like a bug

@dsyme
Copy link
Contributor

dsyme commented Dec 30, 2016

Wow, this is fantastic!

@dsyme
Copy link
Contributor

dsyme commented Dec 30, 2016

  • clean-up API surface - currently PR changes shapes/signatures of existing types and methods which will be a breaking change for anyone who consumed previous version of the compiler. Existing API that deal with string should be reverted back and new API that use Layout should be added instead of replacing existing ones.

This is OK as long as FSharp.Core is stable - for various reasons we have been making pretty regular breaking changes in the surface arera of FSharp.Compiler.Service, and bumping major version numbers on the nuget package when we do.

@dsyme
Copy link
Contributor

dsyme commented Dec 30, 2016

If anyone wants to make some basic improvements like

  • replacing struct/end by [<Struct>]
  • eliding interface/end altogether
  • eliding class/end altogether

etc that would also be great

@vladima
Copy link
Contributor Author

vladima commented Dec 31, 2016

@dotnet-bot test this please

@vasily-kirichenko
Copy link
Contributor

Instance members are indented:

image

But static members sometimes are not:

(ok)
image

(wrong)
image

Actually I've found the only case where it's broken - Async type. The tooltip is huge, maybe it's the root cause.

@vasily-kirichenko
Copy link
Contributor

vasily-kirichenko commented Dec 31, 2016

Another catch - namespace and module keywords are not highlighted:

image

image

@vladima
Copy link
Contributor Author

vladima commented Jan 1, 2017

Colors for module and namespace keywords are fixed. As for indentation I do agree that it is inconsistent with other cases however I'd prefer to fix this in a separate PR. Reasons:

  • technically this is not a regression since it matches existing behavior for types with hidden representation
  • current PR is already quite big and adding more things to it will make its review more complicated.

// cc @KevinRansom and @dsyme for opinions.

@saul
Copy link
Contributor

saul commented Jan 1, 2017

One last issue @vladima, see how the separators look in current master:

image

Note that there are no blank lines around the separator, and there is no separator at the end of the tooltip. See @vasily-kirichenko's first screenshot in his last comment:
image

@cartermp
Copy link
Contributor

cartermp commented Jan 1, 2017

@vladima Agreed re: indentation - unless it's a minor change, adding more improvements to this PR will make it more difficult to review and get merged.

@cartermp
Copy link
Contributor

cartermp commented Jan 1, 2017

@vasily-kirichenko I've noticed the Async tooltip exploding in size in #1973 as well. Perhaps there's some strange code path executed for Async where it's not truncated, because in most situations it cuts off with a ... about 1/3 of the way through listing all the members. I haven't been able to reproduce that kind of behavior for anything other than Async.

@vladima
Copy link
Contributor Author

vladima commented Jan 1, 2017

turned out fix is fairly simple:
image

@dsyme
Copy link
Contributor

dsyme commented Jan 2, 2017

This looks good to me. Unless there are other concerns I think we should accept this

@KevinRansom
Copy link
Contributor

@vladima

Thank you this is awesome work ...

Kevin

@KevinRansom KevinRansom merged commit ed948ab into dotnet:master Jan 3, 2017
@saul saul mentioned this pull request Jan 3, 2017
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…tnet#2070)

* use Layout as a source data of classification related tasks

* fix indentation

* parse xml doc by hand instead of relying on VS service

* revert back old API

* fix portable build

* fix output in FSI

* fix printing of operators

* update Surface test

* internalize new types and modules

* tag module/namespace as keywords

* fix tooltip formatting for types with hidden representation

* fix separator placement
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.

9 participants