Skip to content

gccrs: feat: Made changes to ensure no wrong assignments are done. #3300

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sriganeshres
Copy link
Contributor

gcc/rust/ChangeLog:

* backend/rust-compile-expr.cc (lvalue_p): Created a function that checks the lvalue. (CompileExpr::visit): Modified the function to check the lvalue using the above mentioned function and also checks the type of lvalue expression.

gcc/testsuite/ChangeLog:

* rust/compile/issue-3287.rs: New test for testing wrong assignments.

Fixes #3287

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

There are changes required here i think we can get away wit this check but not totally convinced yet

Also it will need to be applied to the other assignments such as:

x +=1 etc

@philberty philberty added the diagnostic diagnostic static analysis label Dec 11, 2024
@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch 2 times, most recently from 9bd6356 to 5748d31 Compare December 11, 2024 19:53
@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch 4 times, most recently from fc569e4 to 2a551f5 Compare December 14, 2024 16:29
@sriganeshres
Copy link
Contributor Author

There are changes required here i think we can get away wit this check but not totally convinced yet

Also it will need to be applied to the other assignments such as:

x +=1 etc

Can you give me more information about the case we are missing as x+= 1 is valid as long as x is mutable in rust.

@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch 4 times, most recently from 1089291 to e6a88dc Compare December 15, 2024 14:40
@powerboat9
Copy link
Collaborator

There are changes required here i think we can get away wit this check but not totally convinced yet
Also it will need to be applied to the other assignments such as:
x +=1 etc

Can you give me more information about the case we are missing as x+= 1 is valid as long as x is mutable in rust.

You have to adjust the visitor for CompoundAssignmentExpr as well as the visitor for AssignmentExpr

@powerboat9
Copy link
Collaborator

Would probably be good to make lvalue_p operate on HIR nodes instead of trees

@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch 2 times, most recently from c9d729f to caff7b1 Compare January 11, 2025 13:12
@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch from caff7b1 to 4133278 Compare January 15, 2025 16:48
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

could you add a testcase like this one?

fn main() {
    15 = 14;
}

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

LGTM but i think you need to change that test case

Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

Yeah this is good but yeah arthur has some minor comments too which need done.

@sriganeshres sriganeshres force-pushed the feat-wrong-assignment branch 2 times, most recently from 8a274af to 55250bf Compare March 21, 2025 23:03
@sriganeshres sriganeshres requested a review from philberty March 21, 2025 23:04
@sriganeshres
Copy link
Contributor Author

I guess they failing now because of newer tests. If this this merged with upstream there will not be as issue-3147.rs file is added after this changes.

@philberty
Copy link
Member

You need to rebase you code and investigate why you have failing tests.

gcc/rust/ChangeLog:

	* backend/rust-compile-base.cc (HIRCompileBase::is_lvalue): Created a function that
	checks for lvalue.
	* backend/rust-compile-base.h: Created the Signature for above function.
	* backend/rust-compile-expr.cc (CompileExpr::visit): Made changes to ensure
	proper readability and checking for wrong assignments in AssignmentExpr
	and CompoundAssignmentExpr Respectively.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3297.rs: New test (New Feature).
	* rust/compile/issue-3297-2.rs: New test (Regression).
	* rust/compile/issue-3297-3.rs: New test (New Feature).

Signed-off-by: Sri Ganesh Thota <[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 merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gccrs does not check if something is assignable
4 participants