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

Lint 6: duplicate-mutable-accounts #6

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

oslfmt
Copy link
Contributor

@oslfmt oslfmt commented Jul 12, 2022

Quick and somewhat-dirty implementation of lint 6. Tests pass, but may need further work/clean-up when encountering >2 duplicate accounts.

@oslfmt oslfmt requested a review from smoelius July 12, 2022 21:11
@oslfmt
Copy link
Contributor Author

oslfmt commented Jul 16, 2022

Completed upgrade so lint can effectively handle cases where number of identical accounts > 2. All tests pass except for secure. To make secure pass, the lint simply needs to also check for key-check constraints that are not specified within the anchor [#account] macro.

@oslfmt oslfmt force-pushed the 6-duplicate-mutable-accounts branch 3 times, most recently from 8d83daa to d0c2455 Compare July 17, 2022 13:58
/// represent the identifiers being compared. Following the previous example, this would be `(a, b)`.
fn generate_possible_expected_constraints(
identical_types: &[(Symbol, Span)],
) -> Vec<((TokenStream, TokenStream), (Symbol, Symbol))> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add closure argument to do comparison in function to not have to store generated streams

@oslfmt oslfmt force-pushed the 6-duplicate-mutable-accounts branch 2 times, most recently from e42f644 to 12caca7 Compare July 19, 2022 19:14
@oslfmt
Copy link
Contributor Author

oslfmt commented Jul 19, 2022

@smoelius lint 6 is completely done as it stands. Only small thing that is bugging me (and this is personal preference) is that when there are no key checks at all (not using anchor macro constraints, nor alternate constraints such as in function bodies), then the lint will print messages related to the v2 lint I wrote, which doesn't have as nice messages. This is due to some of the logic of the program, and its fairly involved to change it than one might think.

Ideally, if no constraints are in the program at all, I would have liked for the lint to print the v1 messages, ie, the one that recommends adding the #[account] macro constraints, since this is best/recommended practice for anchor programs. I think this applies generally to all these lints, that we should recommend programs to do checks the anchor way, instead of some alternate method.

@oslfmt
Copy link
Contributor Author

oslfmt commented Jul 19, 2022

also if there's too many commits, I can squash

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ smoelius
❌ oslfmt
You have signed the CLA already but the status is still pending? Let us recheck it.

@smoelius
Copy link
Member

@victor-wei126 Can you sign the CLA?

fn is_substream(stream: &TokenStream, other: &TokenStream) -> bool {
let other_len = other.len();
for i in 0..stream.len() {
for (j, other_token) in other.trees().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

I have a hunch this could be written more simply as:

if other.trees().enumerate().all(|(j, other_token)| { ... }) {
    return true;
}

@oslfmt oslfmt force-pushed the 6-duplicate-mutable-accounts branch from e8b0ab4 to bbbb0a9 Compare July 22, 2022 15:08
@smoelius smoelius force-pushed the 6-duplicate-mutable-accounts branch 2 times, most recently from 94b6cf5 to 85a445b Compare August 14, 2022 18:36
@smoelius smoelius force-pushed the 6-duplicate-mutable-accounts branch from 85a445b to e3d777b Compare August 17, 2022 00:37
for next in current + 1..exprs.len() {
if values.check_key_constraint(exprs[current], exprs[next]) {
// if there is at least one alt constraint, set flag to false
self.no_alternate_constraints = false;
Copy link
Member

Choose a reason for hiding this comment

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

This could flip back to true if the program contains more than one function, right? (Or am I misunderstanding?)

I'm wondering if no_alternate_constraints and spans should be maintained per function, similar to how anchor_accounts is maintained per struct def.

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.

3 participants