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

Add std and inlay hints #8

Merged
merged 3 commits into from
Sep 14, 2021
Merged

Add std and inlay hints #8

merged 3 commits into from
Sep 14, 2021

Conversation

HKalbasi
Copy link
Member

I actually done this as an experiment for adding RA to playground but I think this demo can also take advantage of this PR and by merging it, future people don't need to repeat my work.

Fix #7

This satisfies me, but it is not great. Specially it is slower than RA on vscode, and I don't know why it should be that slow. Suggestions on this (and other problems) are welcome.

@bjorn3
Copy link

bjorn3 commented Sep 10, 2021

Could you avoid checking in fake_*.rs and instead add a build step to generate them? They are really big.

async provideInlayHints(model, range, token) {
let hints = await state.inlay_hints();
return hints.map((hint) => {
if (hint.hint_type == 1) {
Copy link

Choose a reason for hiding this comment

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

What about hint type 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

RA doesn't emit type 0 (type 0 is other in lsp/monaco):

                match ih.kind {
                    InlayKind::TypeHint | InlayKind::ChainingHint => InlayHintType::Type,
                    InlayKind::ParameterHint => InlayHintType::Parameter,
                },

Copy link

Choose a reason for hiding this comment

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

Could you remove that enum variant on the rust side? You can explicitly specify a discriminant to keep the numbers identical.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get exactly what you want. InlayHintType is not meaningless and is the rust equivalent of this enum: https://microsoft.github.io/monaco-editor/api/enums/monaco.languages.inlayhintkind.html

You want to send ih.kind and move this match to JS?

Copy link

Choose a reason for hiding this comment

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

If InlayHintType::Other is not handled here I think it is best to remove this variant completely and only re-introduce it once it is being used. I think that will save some debugging when it becomes used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, just the variant. I thought the whole enum should be removed.

www/webpack.config.js Outdated Show resolved Hide resolved
@bjorn3
Copy link

bjorn3 commented Sep 11, 2021

There is a fair amount of commented out code. I think it should be removed before merging.

Copy link

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

I didn't look at rust-pack, but the rest of the code LGTM. This is my first time looking at the code though and I don't have r+ permissions for this repo. r? @matklad ?

@@ -229,7 +247,7 @@ monaco.languages.onLanguage(modeId, async () => {
}
}

monaco.languages.setTokensProvider(modeId, {
/*monaco.languages.setTokensProvider(modeId, {
Copy link

Choose a reason for hiding this comment

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

Did you comment this out as you broke it, or for another reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is broken and for my experiment propose (adding to playground) a static highlighter was enough, so I removed it. But for "rust analyzer demo" we may want to bring it back even if it is broken and worse than static.

Break reproduce:

  1. go to https://ra-wasm.netlify.app/
  2. remove all the code
  3. press ctrl+Z.

@matklad
Copy link
Member

matklad commented Sep 13, 2021

bors r+

Fixed the permissions as well, so r+ should work now.

@bors
Copy link
Contributor

bors bot commented Sep 13, 2021

Configuration problem:
bors.toml: not found

@matklad
Copy link
Member

matklad commented Sep 13, 2021

bors try

bors bot added a commit that referenced this pull request Sep 13, 2021
@HKalbasi
Copy link
Member Author

What happened exactly? I can't see bors fail details.

@lnicola
Copy link
Member

lnicola commented Sep 13, 2021

It didn't run because the trying branch is missing from the workflow.

@HKalbasi
Copy link
Member Author

Seems there is now a trying branch.

bors r=@matklad

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

🔒 Permission denied

Existing reviewers: click here to make HKalbasi a reviewer

@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

I think you need to add it to the workflow on your branch.

bors r=matklad

bors bot added a commit that referenced this pull request Sep 14, 2021
8: Add std and inlay hints r=matklad a=HKalbasi

I actually done this as an experiment for adding [RA to playground](rust-lang/rust-playground#357) but I think this demo can also take advantage of this PR and by merging it, future people don't need to repeat my work.

Fix #7

This satisfies me, but it is not great. Specially it is slower than RA on vscode, and I don't know why it should be that slow. Suggestions on this (and other problems) are welcome.

Co-authored-by: hamidreza kalbasi <[email protected]>
Co-authored-by: HKalbasi <[email protected]>
@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

Yeah, I don't think it works. Can you add it there?

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

Timed out.

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

try

Timed out.

@HKalbasi
Copy link
Member Author

"it" is the bors.toml? I rebased to master if it is.

@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

No, sorry. Add the trying branch to the list at the top of .github/workflows/ci.yaml, under on/push/branches.

@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

bors r=matklad

bors bot added a commit that referenced this pull request Sep 14, 2021
8: Add std and inlay hints r=matklad a=HKalbasi

I actually done this as an experiment for adding [RA to playground](rust-lang/rust-playground#357) but I think this demo can also take advantage of this PR and by merging it, future people don't need to repeat my work.

Fix #7

This satisfies me, but it is not great. Specially it is slower than RA on vscode, and I don't know why it should be that slow. Suggestions on this (and other problems) are welcome.

Co-authored-by: hamidreza kalbasi <[email protected]>
@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

Nope, sorry, it's still not working.

@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

Can you also add staging?

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

Canceled.

@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

bors try

bors bot added a commit that referenced this pull request Sep 14, 2021
@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

bors r=matklad

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

try

Build succeeded:

@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

Build succeeded:

@bors bors bot merged commit 03c4dc6 into rust-analyzer:master Sep 14, 2021
@lnicola
Copy link
Member

lnicola commented Sep 14, 2021

It's up on https://ra-wasm.netlify.app/, and it's awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Std and prelude support
4 participants