-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Amend ink! analyzer (phase 2) #2018
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the amendment. I will share it with the rest of the committee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to highlight that this has been the 2nd amendment with a price increase for this grant and personally, I'm not very keen on potentially approving a 3rd one:
- August: $48k (initial PR)
- September: $54.4k (passed amendment)
- October: $59,6k (this amendment)
Given the fact that you're actively giving updates in the community Polkadot Forum (1, 2) (thx to @semuelle for pointing me to these) which speaks for your commitment to the project, I'm willing to approve this as well, for this time.
@takahser Thanks for the swift review and approval 🙂.
I understand your concern about the price (and scope) increases, but I think both amendments are well justified. I believe at this point the current scope covers ink! v4.3.0 comprehensively, so I don't anticipate any further amendments to the current scope based on ink! v4.3.0 features. However, ink! itself is still in development and there's already a pre-release of ink! v5 that adds a few more attributes and semantic rules. In particular, I'll highlight these:
Adding these at this stage would be premature (as they could change before release), so I ignored them for now, but I also anticipate that ink! v5 (with the above additions) will likely be released before the completion of the current grant. With the project being relatively complex and spread across 3 major components (semantic analyzer, language server and VS Code extension), my current development philosophy is to release features as early as possible (as soon as they work for a few common cases with no false positives) and incrementally improve them over time to provide compressive language support. This allows me to ship useful features to users faster (which should help adoption both ink! analyzer and ink! itself) as opposed to e.g. waiting 6 months before shipping any VS Code features from this grant. I originally believed an amendment (whenever ink! v5 ships) to add a milestone (especially a semantic analyzer focused one, as that's the base component) for well justified new core language features would be relatively straight forward and uncontroversial. But I'm not quite sure about that now given the above comment. It would be interesting to hear thoughts about how to best handle that case whenever it arises given the above context. Thanks again for the approval, the above just helps me understand how to better think about future updates and/or amendments 🙂. Side note: @semuelle thanks for sharing the forum posts! I should have a new one soon (end of this week or early next week) announcing new features.🙂 |
Thanks @davidsemakula I'll let @takahser speak for himself, but in my personal opinion, (to answer your question) rather than having one grant that stretches out for a long time, I think this could be accomplished through a follow-up grant instead of an amendment. That might make it easier to justify the pricing as well. Also, if the v5 updates end up being rather straight-forward and easy to implement, a level 1 grant would only need 2 approvals. Just a thought for the future. |
Thanks @keeganquigley for the quick response and thoughts. I completely understand where both you and @takahser are coming from, and from a high level perspective, I'd probably have the same initial comments if I was an evaluator 🙂. I just wanted to add more nuance as to why I was favoring the amendment approach (for this specific grant). I certainly don't think a grant should go on indefinitely! 🙈 In fact, if not for the ink! v5 features, I simply would have said:
And that would have been the end of my response. However, whenever v5 is released, this would be the "hypothetical" difference with the 2 options:
In reality (for practical reasons), regardless of an amendment (i.e. the follow-up grant scenario), at the very least, minimal support for v5 syntax would be added anyway through the course of this grant, as releasing new versions while ignoring basic annoyances due to ignoring v5 syntax probably wouldn't be great for adoption. But it would be great (from my perspective) for additional development to be happening with some support. Again, just adding a bit more nuance to the discussion for the context of this specific grant. 🙂 |
@davidsemakula sounds good, ultimately it's up to you how you want to proceed. Thanks for the clarifications and keep up the great work! |
If you are planning to continuously add features and improve upon the analyzer (we are glad to see this, btw), @davidsemakula, it would be a perfect candidate for a maintenance grant. That way, we wouldn't have to approve every feature change. Your current grant still covers quite a long roadmap (6m+), so fitting that in between won't be straightforward, but at least something we should keep in mind. |
Project Abstract
This is an amendment to the ink! Analyzer (phase 2) grant.
Scope:
These updates modify the scope/deliverables for milestones 3 and 4.
The rest of the milestones remain unchanged as (on balance) their scope is not too significantly affected by these changes.
Updates:
Milestone 3:
I've found that while paths as argument values in attributes were custom ink! specific syntax at the time the ink_ir crate was implemented, they're now part of the "meta item" attribute syntax used by Rust's built-in attributes (or more technically, arbitrary expressions - including path expressions - are now allowed as the
value
part ofkey=value
segments for attributes at parse time).This means any fixes (where necessary) for generic language support features (e.g. go to definition, find references, rename e.t.c) for path-based argument values belong in generic Rust tools (e.g. rust-analyzer and IntelliJ Rust) not ink! analyzer. In this case, however, rust-analyzer already has good support while IntelliJ Rust is lacking support for some features (but that's out of scope for this particular grant/ current IDE targets).
I've also created a PR for the ink! repository (specifically the ink_ir crate's ast module) that: updates related documentation to more accurately describe ink! attribute argument syntax and its current deviations from of Rust "meta item" attribute syntax, and simplifies the implementation of ink! attribute meta parsing implementation by removing unnecessary custom utilities (where appropriate).
Conversely, I've also found that diagnostics and quickfixes support for ink! trait definition implementations is worse than anticipated in generic Rust tools. Specifically, I expected both rust-analyzer and IntelliJ Rust to provide diagnostics and quickfixes for missing methods/members for ink! trait definition implementations (as they do for "normal" Rust trait implementations). However, rust-analyzer provides neither, while IntelliJ Rust provides only diagnostics but no useful quickfixes (i.e. it's aware that methods are missing but doesn't provide correct quickfixes for the issue), and the issue doesn't lend itself well to a generic fix (see previous references to the challenges generic IDE tools have with macro expansion/name resolution and trait resolution).
So I've updated milestone 3 to incorporate these findings by:
impl Environment
.Milestone 4:
In the previous amendment, I missed a few more traits that chain extension error types (i.e.
ErrorCode
type) and utility types (i.e. input and output types of associated methods) are required to implement. These are specifically SCALE codec traits (i.e. scale'sEncode
andDecode
and scale-info'sTypeInfo
traits).This amendment adds 2 deliverables to milestone 4 for adding diagnostics, quickfixes and code/intent actions for implementing these traits for 1) the
ErrorCode
type and 2) all input and output types of chain extensions methods where necessary.https://use.ink/macros-attributes/chain-extension#structure
https://use.ink/macros-attributes/chain-extension#example-definition
Grant level
Application Checklist
project_name.md
).@_______:matrix.org
(change the homeserver if you use a different one)