Skip to content

Add Solidity language#7152

Closed
tomholford wants to merge 2 commits intozed-industries:mainfrom
tomholford:th/solidity
Closed

Add Solidity language#7152
tomholford wants to merge 2 commits intozed-industries:mainfrom
tomholford:th/solidity

Conversation

@tomholford
Copy link
Contributor

@tomholford tomholford commented Jan 31, 2024

Context

This adds support for Solidity to zed

Resolves zed-industries/extensions#160

Preview

Syntax Highlighting

image

Go To Definition

go-to-definition

Auto-Formatting

formatting

Tooltips

tooltip

@cla-bot
Copy link

cla-bot bot commented Jan 31, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @tomholford on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@maxdeviant maxdeviant changed the title devex: solidity language Add Solidity language Jan 31, 2024
@tomholford
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 31, 2024
@cla-bot
Copy link

cla-bot bot commented Jan 31, 2024

The cla-bot has been summoned, and re-checked this pull request!

@tomholford tomholford force-pushed the th/solidity branch 2 times, most recently from 96c387d to e8067ab Compare February 1, 2024 23:27
@lutfi-haslab
Copy link

When this feature release?

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

I'm trying to run this on https://github.com/SunWeb3Sec/DeFiHackLabs (repo chosen arbitrarily) and see almost zero LSP functions:

  • completions are very sparse and seem not to allow to complete what the code around uses
    image

  • no go to definition seem to work at all

  • same for renames

  • at least, LSP server downloads fine and seem to format the project fine

Have I done something wrong, is it a regular experience with Solidity LSP or can we make it better?
https://github.com/NomicFoundation/hardhat-vscode/tree/development/server states all those features.

@SomeoneToIgnore SomeoneToIgnore marked this pull request as draft February 6, 2024 08:42
@tomholford
Copy link
Contributor Author

tomholford commented Feb 6, 2024

@SomeoneToIgnore: Thanks for marking as draft. Quick update: Had to focus on other priorities, but will be able to pivot back to this soon.

At this point I have some of the LSP features working: go to definition, basic type tooltips, auto-formatting, syntax highlighting. I'm going to try to take another crack at getting the advanced features mentioned in the solidity-language-server docs working.

I have pushed a wip commit with my progress so far. I am new to this project (and Rust) and could use help if you have a moment. Do you happen to know why the print_completion_item closure is not printing any output when I run the binary in dev mode?

@SomeoneToIgnore
Copy link
Contributor

Great news!
I'm afraid I have no idea why print_completion_item does not print, but do you say that non-dev mode works just fine?

@tomholford tomholford force-pushed the th/solidity branch 5 times, most recently from eacd1ef to 0cbd5e9 Compare February 7, 2024 17:51
@tomholford
Copy link
Contributor Author

Great news! I'm afraid I have no idea why print_completion_item does not print, but do you say that non-dev mode works just fine?

@SomeoneToIgnore I have rebased on main, run in both dev and non-dev modes, and the features highlighted in the PR description are working as expected. I think this is a good first cut for Solidity integration with zed. A future iteration could add the advanced features / deeper integration with the Solidity LSP. So, going to re-open this for review.

@tomholford tomholford marked this pull request as ready for review February 7, 2024 17:59
@maxdeviant
Copy link
Member

This is going to have some conflicts with #7467, so we're going to land that first.

@tomholford
Copy link
Contributor Author

tomholford commented Feb 7, 2024

This is going to have some conflicts with #7467, so we're going to land that first.

@maxdeviant Thanks for the heads up. Now that #7467 is merged, I rebased this branch on main and made the appropriate changes to this diff. Tested locally with both cargo run and cargo run --release and still working as expected. So, requesting a re-review.

edit: cc @SomeoneToIgnore

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

I have not noticed any QOL improvements in the project I linked above since the last review:

  • zero rename, tooltips or go to definition capabilities still
  • while the completion list got bigger, it seems that now it just tries to add every possible thing to the completion list?
    E.g. here it allows me to autocomplete every "struct" after the . which does not seem right at all:
    image

Is there a public project that I can try where something from above works?

@koloz193
Copy link

koloz193 commented Feb 8, 2024

I have not noticed any QOL improvements in the project I linked above since the last review:

  • zero rename, tooltips or go to definition capabilities still
  • while the completion list got bigger, it seems that now it just tries to add every possible thing to the completion list?
    E.g. here it allows me to autocomplete every "struct" after the . which does not seem right at all:
    image

Is there a public project that I can try where something from above works?

do you mean a different solidity repo or a different editor with autocomplete? if the latter, i use vscode which doesnt really support great autocomplete (if any at all). i think that even having syntax highlighting and error indication would satisfy most people, although full support is def better

@SomeoneToIgnore
Copy link
Contributor

Great point about VSCode, I've tried to use https://marketplace.visualstudio.com/items?itemName=JuanBlanco.solidity extension and found its functionality quite morbid too.

Speaking of

error indication

I've noticed that VSCode's plugin is able to show those, yet Zed's does not at all?
image

Also notice that there's hovers shown too, but those seem quite useless information-wise.


I would still want get down to the bottom of #7152 (comment) comment:

and the features highlighted in the PR description are working as expected

I see no "go to definition" and hovers for https://github.com/SunWeb3Sec/DeFiHackLabs but see those in the PR description.
What is wrong here? The project in question? Me trying the wrong definitions for those features?
Before merging this, I would love to experience myself that the features declared in the PR description work.

@stevennevins
Copy link

Great point about VSCode, I've tried to use https://marketplace.visualstudio.com/items?itemName=JuanBlanco.solidity extension and found its functionality quite morbid too.

Speaking of

error indication

I've noticed that VSCode's plugin is able to show those, yet Zed's does not at all? image

Also notice that there's hovers shown too, but those seem quite useless information-wise.

I would still want get down to the bottom of #7152 (comment) comment:

and the features highlighted in the PR description are working as expected

I see no "go to definition" and hovers for https://github.com/SunWeb3Sec/DeFiHackLabs but see those in the PR description. What is wrong here? The project in question? Me trying the wrong definitions for those features? Before merging this, I would love to experience myself that the features declared in the PR description work.

For feature parity I would compare with this which uses the same LSP under the hood

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Feb 8, 2024

Thank you, that makes things more comparable indeed!
At least, the formatting functionality and zero error highlights behavior matches now.

Yet, that plugin shows hovers and allows to go to definition in the project I use:
image

and current PR's version does not, so my questions in the previous message hold.

@tomholford
Copy link
Contributor Author

Thank you, that makes things more comparable indeed! At least, the formatting functionality and zero error highlights behavior matches now.

Yet, that plugin shows hovers and allows to go to definition in the project I use: image

and current PR's version does not, so my questions in the previous message hold.

Thanks for the feedback. Any suggestions on how to proceed from here? Tried to print debug output to the console, but no luck with that.

@maxbrunsfeld
Copy link
Collaborator

Just a heads up - we are very soon going to be moving most of Zed's built-in languages into installable extensions (#7096). Solidity is a major enough language that I'm ok with merging this in the meantime. But once the extension system lands, we will probably remove this code and put it into a new solidity repository under the new zed-extensions GitHub org. We'll probably add you as a collaborator on that repo when we do it, if you're interested in helping maintain it.

It would be good to understand why some of the LSP features aren't working for @SomeoneToIgnore . @tomholford so just to confirm - the LSP works for you reliably? What Solidity projects are you testing against?

@tomholford
Copy link
Contributor Author

tomholford commented Feb 12, 2024

Just a heads up - we are very soon going to be moving most of Zed's built-in languages into installable extensions (#7096). Solidity is a major enough language that I'm ok with merging this in the meantime. But once the extension system lands, we will probably remove this code and put it into a new solidity repository under the new zed-extensions GitHub org. We'll probably add you as a collaborator on that repo when we do it, if you're interested in helping maintain it.

Thanks for the heads up. And that sounds good, I'm happy to help any way that I can.

It would be good to understand why some of the LSP features aren't working for @SomeoneToIgnore . @tomholford so just to confirm - the LSP works for you reliably? What Solidity projects are you testing against?

Confirming that the features described in the PR's Preview section above WOMM. I have been using my fork (with periodic rebases on main) for other projexts. As a test project to record the gifs above, I have been using this:

https://github.com/PopPunkLLC/gaslite-core

@SomeoneToIgnore
Copy link
Contributor

I see that things like Java are being added as Zed extensions: zed-industries/extensions#57
There seems to be slightly less nitpicking in reviews for such PRs (at least, I'm not reviewing anything there, so won't complain about anything 🙂 ) as those are considered "standalone" from Zed code.

Maybe, it's worth converting this into the extension and submitting a PR there.

@@ -0,0 +1,43 @@
;; Method and Function declarations
(contract_declaration (_
(function_definition
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zed doesn't use tags.scm. Could you remove this one?

@tomholford
Copy link
Contributor Author

I see that things like Java are being added as Zed extensions: zed-industries/extensions#57 There seems to be slightly less nitpicking in reviews for such PRs (at least, I'm not reviewing anything there, so won't complain about anything 🙂 ) as those are considered "standalone" from Zed code.

Maybe, it's worth converting this into the extension and submitting a PR there.

Good call, going to close this and reopen as an extension.

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

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Solidity

7 participants