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

gccrs does not check if something is assignable #3287

Open
1 task
liushuyu opened this issue Dec 3, 2024 · 7 comments · May be fixed by #3300
Open
1 task

gccrs does not check if something is assignable #3287

liushuyu opened this issue Dec 3, 2024 · 7 comments · May be fixed by #3300
Assignees
Labels
diagnostic diagnostic static analysis

Comments

@liushuyu
Copy link
Contributor

liushuyu commented Dec 3, 2024

Summary

gccrs does not check if something is assignable, and gccrs accepts many un-assignable expressions.

Reproducer

I tried this code:

fn main() -> i32 {
    let mut x = 42;
    x + 1 = 2;

    x
}

Does the code make use of any (1.49) nightly feature ?

  • Nightly

Godbolt link

No response

Actual behavior

The current behavior is that gccrs accepts this kind of semantically invalid code and even successfully generates machine code for them (in this case, gccrs generates code to make the function return 42).

Expected behavior

I expected to see an error thrown like rustc does:

error[E0070]: invalid left-hand side of assignment
 --> test1.rs:3:11
  |
3 |     x + 1 = 2;
  |     ----- ^
  |     |
  |     cannot assign to this expression

GCC Version

fa93e28

@philberty
Copy link
Member

I've purposely not tackled this because i have a feeling implementing a walk tree function like we do inside:

https://github.com/Rust-GCC/gccrs/blob/master/gcc/rust/checks/lints/rust-lint-unused-var.cc

and looking for the assignment expressions and checking the const qualifiers might be enough. But it probably wont be perfect it needs some though with someone with free time

@philberty
Copy link
Member

There is a better way to implement this by copying some of the code from the c front-end: We need a check in our assignment expression during code generation which is here:

Where we need to port over this function from the c front-end:

lvalue_p (const_tree ref)

This will tell us true or false if its a valid lvalue so then if its not we can emit a rust_error_at diagnostic.

@philberty philberty added this to the Remaining typecheck issues milestone Dec 10, 2024
@philberty philberty added the diagnostic diagnostic static analysis label Dec 10, 2024
@CohenArthur
Copy link
Member

how complicated would that be compared to making a simple visitor on our side? I feel like there's two different things, checking if it's a proper lvalue and checking if it's mutable - both of which would be implemented super quickly with one of our custom visitors

@philberty philberty self-assigned this Dec 11, 2024
@philberty
Copy link
Member

Yeah i think its two issues that need seperated

@philberty
Copy link
Member

part of the issue is the overflow traps return a var decl which is what we want so i think we need to use the lvalue check from c fropnmt-end and something on our HIR as well where yes GCC wise its a var devel but this HIR expr is an operator so its definetly not an lvalue

@philberty
Copy link
Member

I sort of think another tree walker could do the assignment check and look if the lvalue is TREE_READONLY but not totally sure it handles it properly for rust

philberty added a commit that referenced this issue Dec 18, 2024
This ensures that we handle var decls readonly checks much better

Addresses: #807 #3287

gcc/rust/ChangeLog:

	* checks/errors/rust-readonly-check.cc (check_decl): improve mut check
	(emit_error): helper
	(check_modify_expr): likewise
	(readonly_walk_fn): reuse helper
	(ReadonlyCheck::Lint): cleanup context each run

gcc/testsuite/ChangeLog:

	* rust/execute/torture/builtin_macro_include_bytes.rs: missing mut

Signed-off-by: Philip Herron <[email protected]>
@philberty
Copy link
Member

this solves the mutability checks for this case:

#3311

philberty added a commit that referenced this issue Dec 18, 2024
This ensures that we handle var decls readonly checks much better

Addresses: #807 #3287

gcc/rust/ChangeLog:

	* checks/errors/rust-readonly-check.cc (check_decl): improve mut check
	(emit_error): helper
	(check_modify_expr): likewise
	(readonly_walk_fn): reuse helper
	(ReadonlyCheck::Lint): cleanup context each run

gcc/testsuite/ChangeLog:

	* rust/execute/torture/builtin_macro_include_bytes.rs: needs mut
	* rust/compile/mutability_checks1.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostic diagnostic static analysis
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants