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.

Sign up for GitHub

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

Jump to bottom

Add type descriptions to completions and hovers, support method hovers. #1915

Merged
merged 10 commits into from
Apr 9, 2021
Merged

Add type descriptions to completions and hovers, support method hovers. #1915

merged 10 commits into from
Apr 9, 2021

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Mar 17, 2021 •
edited

Closes #668
Closes #2074

Screen-Recording-2021-04-02-at-1

  • Add type property descriptions to hovers and completions
  • Changes to type system & binding so that object properties & methods are dynamically resolved and not bound upfront.

NOTE: This PR does not include an implementation for find references on object property symbols.

@miqm miqm mentioned this pull request Mar 21, 2021
Merged
9 tasks
@anthony-c-martin anthony-c-martin mentioned this pull request Mar 22, 2021
Merged
@anthony-c-martin anthony-c-martin force-pushed the antmarti/method_hovers branch 3 times, most recently from d01ee34 to bb7f937 Compare April 3, 2021 03:49
@anthony-c-martin anthony-c-martin changed the title [WIP] Use method resolver for method hovers Add type descriptions to completions and hovers, support method hovers. Apr 6, 2021
@anthony-c-martin anthony-c-martin force-pushed the antmarti/method_hovers branch 2 times, most recently from 9ce98a5 to 6d4a78a Compare April 7, 2021 17:47
@anthony-c-martin anthony-c-martin mentioned this pull request Apr 8, 2021
Closed
@anthony-c-martin anthony-c-martin marked this pull request as ready for review April 8, 2021 13:59
miqm
miqm reviewed Apr 8, 2021
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
shenglol
shenglol reviewed Apr 8, 2021
View reviewed changes
src/Bicep.Core.Samples/Files/InvalidVariables_LF/Completions/objectVarTopLevel.json Outdated
[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems odd. Any idea why the completion items are removed?

Copy link
Member Author

@anthony-c-martin anthony-c-martin Apr 8, 2021 •
edited

Choose a reason for hiding this comment

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

Good catch. I think because of #1915 (comment) - we use the declared type rather than assigned type for member access completions. I may need to revise that approach...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah...

Copy link
Member Author

@anthony-c-martin anthony-c-martin Apr 9, 2021 •
edited

Choose a reason for hiding this comment

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

2440252 - here's is a more targeted fix I came up with to avoid the infinite recursion. I don't love it because it feels very special-case.

I did this because:

  • language server functionality depends on the declared type for variable declarations being resolved (rather than just any)
  • we otherwise have infinite recursion (variable declared type -> variable value assigned type -> for loop assigned type -> object assigned type -> object declared type -> for loop declared type -> variable declared type)

Other potential fixes I thought of:

  • removing the language server's dependency on declared type (big change, which I don't want to make with this PR).
  • passing flags to GetDeclaredType() to shortcut recursion when called internally (by returning any), but keeping the existing behavior when called externally.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I prefer

removing the language server's dependency on declared type

to be the ultimate fix because that avoid special cases, but within the scope of this PR the fix looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I feel like this can be a question for Anders.

majastrz
majastrz requested changes Apr 8, 2021
View reviewed changes
Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

Added some comments.

@majastrz
Copy link
Member

majastrz commented Apr 8, 2021

I tried getting a hover for a discriminator property and didn't get anything:
image

But it worked for a regular property:
image

Same problem in a non top-level discriminator property as well:
image

It may be difficult to produce a description for that property depending on how the swagger is structured for that type, but we should show something similar to what the completion text looks like for the same property.

@majastrz
Copy link
Member

majastrz commented Apr 8, 2021

Btw, I'm assuming that find refs for properties is out of scope of this PR?

@anthony-c-martin
Copy link
Member Author

Btw, I'm assuming that find refs for properties is out of scope of this PR?

Yes. I've added that as a note to the description. This implementation simply generates a new PropertySymbol when requested with no caching. Find refs would need a cache layer, plus means answering some tough questions about what we consider equivalent when comparing properties.

@anthony-c-martin
Copy link
Member Author

I tried getting a hover for a discriminator property and didn't get anything:

Good find! I've fixed the hover and added a test for it.

As you mention, showing a description is a little tricky. I think we have all the information we need in swagger, but we currently store descriptions in the individual property fields - meaning we only have descriptions for the sub class discriminator properties and not the base class discriminator property. I'll add an issue to the types repo for this.

shenglol
shenglol approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

This is awesome!

majastrz
majastrz approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

This is great!

@majastrz majastrz merged commit bdbaa87 into main Apr 9, 2021
@majastrz majastrz deleted the antmarti/method_hovers branch April 9, 2021 17:57
@miqm
Copy link
Collaborator

miqm commented Apr 9, 2021

Seems like #1571 is unblocked, I’ll bring my branch up2date sometime next week.

@anthony-c-martin
Copy link
Member Author

Seems like #1571 is unblocked, I’ll bring my branch up2date sometime next week.

I was actually partway through merging main into your branch; I'll send you a PR if I get there, so you can choose whether or not you want to accept it.

@miqm
Copy link
Collaborator

miqm commented Apr 9, 2021

Seems like #1571 is unblocked, I’ll bring my branch up2date sometime next week.

I was actually partway through merging main into your branch; I'll send you a PR if I get there, so you can choose whether or not you want to accept it.

awesome! thanks :)

@anthony-c-martin
Copy link
Member Author

awesome! thanks :)

@miqm I pushed accidentally rather than submitting a PR. Tests are passing though, which is a good sign!

This was referenced Apr 19, 2021
Closed
Merged
Closed
@anthony-c-martin anthony-c-martin mentioned this pull request Jul 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miqm miqm miqm left review comments

@shenglol shenglol shenglol approved these changes

@majastrz majastrz majastrz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for method & property hovers Add descriptions to generated types
4 participants
@anthony-c-martin @majastrz @miqm @shenglol