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

Arbitrary functions shouldn't be isomorphic to tuple structs #10200

Closed
pythonesque opened this issue Oct 31, 2013 · 8 comments · Fixed by #18171
Closed

Arbitrary functions shouldn't be isomorphic to tuple structs #10200

pythonesque opened this issue Oct 31, 2013 · 8 comments · Fixed by #18171
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️

Comments

@pythonesque
Copy link
Contributor

Currently, if we define a tuple struct:

struct S(t1, ..., tn)

then S has the same type as

fn( x1 : t1, ..., xn : tn) -> S

This allows us to write code that seems unintuitive / wrong:

struct Foo(bool);
let x = Foo(true);
fn foo(_: uint) -> Foo { Foo(false) } // Note that we don't even check to make sure the argument types match in patterns

match x {
    foo(x) => println!("{}", x)
}
// Produces `true`

It seems to me that we should disallow this. My preference would be to just make tuple structs unique, named type constructors that wrap tuple type constructors (not functions). So the type of S above might be (internally) Unique(Tuple(t1, ..., tn), S).

@brson
Copy link
Contributor

brson commented Nov 1, 2013

It's certainly a feature that we never use.

@pythonesque
Copy link
Contributor Author

The root cause is b50aa82. So we need to figure out how to handle tuple struct exports properly--I'm not sure of exactly what that would entail, @luqmana might know more.

@alexcrichton
Copy link
Member

I'm a little confused to what the problem is. I personally really like the fact that an enum tuple variant or a struct tuple has its name as a functional constructor. It's not something that I used that much, but I really enjoy doing it when I do use it.

This code should not be passing the typechecker for sure. We don't allow arbitrary code to be run inside of a pattern, so the foo(x) should not compile at all. It looks like the typechecker is matching patterns based on types and Foo just happens to have the same type as foo (although that's just a guess)

@pythonesque
Copy link
Contributor Author

See above. The code passes the typechecker because, as you said, foo and Foo have the same types--more generally, because we export tuple structs as functions so they are allowed to exist anywhere a tuple struct could exist. The problem is that the results it gives are totally inconsistent with what you would expect in this situation--it doesn't run arbitrary code, it just treats it like Foo.

In order to make that not happen, I can't really see any way around either
(1) explicitly marking tuple structs as different from ordinary functions, or
(2) in general, finding some other property that is shared by tuple structs and things allowed in match patterns, like maybe figuring out if it's a constant expression.

I think (1) is simplest but at that point we're essentially acknowledging that tuple structs aren't just functions, even when they look like functions. If we do that but still let you use them as functions in other contexts, we're basically just providing coercion for a really unusual use case (especially if we do what we should be doing and compile down tuple structs to not be function calls wherever possible, as per #7435).

Right now, defining a tuple struct creates both a type and a value which happen to have the same name, which seems rather confusing to me pedagogically as well. To me, the added confusion doesn't really seem worth the very rare situation where we actually do want to treat them as functions, particularly since we can just use a closure in those situations.

@alexcrichton
Copy link
Member

I don't think that forbidding tuple structs/enum tuples from being functions is the fix for this problem.

In the above code, foo(x) is not a valid pattern, you're matching against something of type Foo which means that the binding name for the sub-pattern-matching must be Foo (or perhaps also something which resolved to Foo). In this case you're using foo which is a function and has no relation to Foo at all.

I don't think that this use case warrants removal of Foo being a function (because it's a different bug).

@pythonesque
Copy link
Contributor Author

I'm using foo(x) which is a value of type Foo. It has the correct arity, and returns the correct value, so as far as the typechecker is concerned it is indeed a valid pattern (note that I changed the example to use uint to demonstrate that last point but even if it did check that the types of the arguments were correct that wouldn't be enough to disallow foo). The only way you can disallow foo in general is to disallow all functions in match patterns, or allow some functions in match patterns and evaluate them properly. We can't do the latter right now, and we can't do the former unless b50aa82 is resolved in a way that differentiates tuple structs from regular functions.

As far as your last point, yes, we can fix this without removing Foo as a function--but not without making tuple structure functions "special" in some way. If you disallow all functions in match patterns, then we don't have to represent it internally as a function at all unless it's cast as one, but then that use case just becomes type coercion from tuple structs to functions, which is what I was saying.

@nikomatsakis
Copy link
Contributor

I don't think we should remove the fact that variant and struct constructors are functions, we should just change the pattern matching code to insist that the path is resolved to a variant or struct name. I'm rather surprised this is not how it works now but I'd have to look at the code to see why.

@nikomatsakis
Copy link
Contributor

From discussions on IRC, it seems that this problem only occurs with structs, not enum variants (which makes sense...) and the reason is because, in the cross-crate metadata, we encode a struct ctor as a normal fn, rather than giving it a distinct family (the same may be true in the intracrate case, I did not investigate)

@ghost ghost added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Oct 18, 2014
bors added a commit that referenced this issue Oct 25, 2014
Rather than doing it top-down, with a known expected type, we will now simply establish the appropriate constraints between the pattern and the expression it destructures.

Closes #8783.
Closes #10200.
flip1995 pushed a commit to flip1995/rust that referenced this issue May 20, 2023
Rename `integer_arithmetic`

The lack of official feedback in rust-lang#10200 made me give up on pursuing the matter but after yet another use-case that is not handled by `integer_arithmetic` (rust-lang#10615), I think it is worth trying again.

---

changelog: Move/Deprecation: Rename `integer_arithmetic` to `arithmetic_side_effects`
[rust-lang#10674](rust-lang/rust-clippy#10674)
<!-- changelog_checked -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants