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

fix(proc macros): don't use params as name #1363

Merged
merged 4 commits into from
Apr 29, 2024
Merged

fix(proc macros): don't use params as name #1363

merged 4 commits into from
Apr 29, 2024

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Apr 29, 2024

Because jsonrpsee's internal proc macro code was using params as internal variable name it could conflict and with params in the user's API in such case it may try to overwrite the "wrong" variable.

In this PR I have renamed this internal variable to something much weirder which should be quite unlikely that anyone will use and panic if those collide to make it easier to debug.

Close #1362

Because jsonrpsee's internal proc macro code was using `params` it could
conflict and with `params` in the user's API in such case it
may try to overwrite the "wrong" variable.

In this PR I have renamed this internal variable to something much
weirder which should be quite unlikely that anyone will use.
@niklasad1 niklasad1 requested a review from a team as a code owner April 29, 2024 13:14
@niklasad1
Copy link
Member Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review

⏱️ Estimated effort to review [1-5]

3, because the changes involve understanding the context of macro usage and ensuring that the renaming and error handling logic are correctly implemented without introducing new issues.

🧪 Relevant tests

No

🔍 Possible issues

Possible Bug: The code checks for conflicts with the ILLEGAL_PARAM_NAME by comparing against the string representation, but it should compare against the Ident representation directly. This might not catch all potential conflicts.

🔒 Security concerns

No

Code feedback:
relevant fileproc-macros/src/render_client.rs
suggestion      

Consider using a more robust method for generating a unique identifier rather than a fixed string. This could involve using a UUID or a similarly unique generator to minimize the risk of name collisions further. [important]

relevant lineconst ILLEGAL_PARAM_NAME: &str = "__________________params";

relevant fileproc-macros/src/render_client.rs
suggestion      

Instead of panicking directly when a name collision is detected, it might be better to return an error from the function. This would allow the caller to handle the error more gracefully. [important]

relevant linepanic!("Cannot use `{}` as a parameter name", ILLEGAL_PARAM_NAME);

relevant fileproc-macros/src/render_client.rs
suggestion      

To ensure that the identifier comparison is accurate, compare Ident directly rather than converting both to a string. This avoids issues with different representations leading to false negatives. [important]

relevant lineif param_names.iter().any(|name| name == ILLEGAL_PARAM_NAME) {

relevant fileproc-macros/src/render_client.rs
suggestion      

Add a unit test to verify that the new logic correctly handles scenarios where the user's API might use the ILLEGAL_PARAM_NAME. This is crucial to ensure that the panic behavior works as expected. [important]

relevant lineif param_names.iter().any(|name| name == ILLEGAL_PARAM_NAME) {


✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

  • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

@@ -210,6 +210,8 @@ impl RpcDescription {
param_kind: &ParamKind,
signature: &syn::TraitItemFn,
) -> TokenStream2 {
const ILLEGAL_PARAM_NAME: &str = "__________________params";
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only wee thing is that this might provoke some sort of clippy warning about _ being used in front of a variable that's in use, but perhaps not an issue (and you could call it eg jsonrpsee__params or whatever if it was) :)

Copy link
Member Author

@niklasad1 niklasad1 Apr 29, 2024

Choose a reason for hiding this comment

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

I double checked and async trait ignores the let _x = .. warning but I also changed this to camelCase which will be a warning if someone is using it in their API.

I also ignore's to the related lints this to be on the safe-side.

@niklasad1 niklasad1 merged commit 925a7d3 into master Apr 29, 2024
11 checks passed
@niklasad1 niklasad1 deleted the na-fix-1362 branch April 29, 2024 15:18
niklasad1 added a commit that referenced this pull request Apr 29, 2024
* fix(proc macros): don't use `params` as name

Because jsonrpsee's internal proc macro code was using `params` it could
conflict and with `params` in the user's API in such case it
may try to overwrite the "wrong" variable.

In this PR I have renamed this internal variable to something much
weirder which should be quite unlikely that anyone will use.

* fix nit

* allow snake case and underscore

* better error message
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.

Can't use params as parameter name when using subscription proc-macro attribute
3 participants