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

Result depending on execution order #277

Open
1 of 3 tasks
llogiq opened this issue Sep 2, 2015 · 5 comments
Open
1 of 3 tasks

Result depending on execution order #277

llogiq opened this issue Sep 2, 2015 · 5 comments
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST T-MIR Type: This lint will require working with the MIR

Comments

@llogiq
Copy link
Contributor

llogiq commented Sep 2, 2015

In Rust issue #28160, Niko shows an example containing the line x += { x = 20; 2 }, and asks the question what execution order should be used.

I think we should have a lint against those degenerate cases. However, I'm not sure what patterns we should match.

issues left:

  • simple cases with one variable
  • struct/tuple fields (easy)
  • taking a reference to a field stops the lint from checking any further, even if it is dereferenced again (medium)
@Manishearth Manishearth added good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST A-lint Area: New lints labels Sep 2, 2015
@Manishearth
Copy link
Member

Marking as easy, but a more involved (medium difficulty) check is also possible using visitors.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 11, 2016

The simple cases are caught now, added list of remaining things.

@oli-obk oli-obk reopened this Aug 11, 2016
@oli-obk oli-obk added the E-medium Call for participation: Medium difficulty level problem and requires some initial experience. label Aug 11, 2016
@scurest
Copy link
Contributor

scurest commented Aug 12, 2016

Actually, I forgot that Rust will let you take the address of a temporary by prolonging its lifetime. So although I was thinking &{...; x} would somehow take the address of x, of course it only copies x and takes the address of that copy. So the final tickbox is easy; when you find the variable, just check if the parent expression is ExprAddrOf and don't lint if it is. The second box is the harder one :D

So to check, is there any case where a variable appears in an expression but isn't read other than

  • on the left side of an assignment; x = ...
  • directly under a &; &x or &mut x, or
  • inside of a closure?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 12, 2016

What about x = (x+=1, *&x).1? Won't the reference stop the linting, too?

@scurest
Copy link
Contributor

scurest commented Aug 13, 2016

Yes, but generally it's hard to decide whether the address is dereferenced! For example, (x = 1, p = &x) is fine, but (x = 1, {p = &x; q = p; *q}) is not and whether

(x = 1, {
    a[0] = &x;
    *a[/*arbitrary expression */]
})

is fine is undecidable. Similarly for closures ((x = 1, c = || { x = 2 }) is fine...). So I was just planning on letting those pass. A more complicated check is definitely more difficult though.

@oli-obk oli-obk added the T-MIR Type: This lint will require working with the MIR label Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST T-MIR Type: This lint will require working with the MIR
Projects
None yet
Development

No branches or pull requests

5 participants