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

Use Declarations in More Places #1976

Closed
wants to merge 4 commits into from

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented Apr 18, 2017

Rendered

I personally care more about use in impl than in match, but don't see why we cannot really have both, other than potential syntactical ugliness.

@lambda-fairy
Copy link
Contributor

I like having use in impl, but having it in match feels weird to me. In particular, we'd have to allow code like this:

match self.0.partial_cmp(other.0) {
    None => unreachable!(),
    Some(Greater) => Some(Lesser),

    use Ordering::*;  // 😣

    Some(Less) => Some(Greater),
    Some(Eq) => Some(Eq)
}

@burdges
Copy link

burdges commented Apr 18, 2017

There is a case for struct, enum, and trait as well, although struct and enum come with the same concerns as match. Yes impl is the big offender here.

@Havvy
Copy link
Contributor Author

Havvy commented Apr 18, 2017

Over IRC @nagisa suggested use <foo> in ITEM|EXPR as an alternative.

Having it in trait also makes sense for the same reason as having it in impl. If we go the route of having it in match like as describe, it would also make sense to have it in struct (though not tuple structs) and enum as well...I'll add them to the RFC.

For having use in the middle of the match like in @lfairy 's example, that's sort of just as weird as doing in modules today where you use in the middle of some functions have have functions prior using those bindings.

@mdinger
Copy link
Contributor

mdinger commented Apr 18, 2017

The match example seems related to issue #421 and to PR #1949

@Havvy Havvy changed the title Use Declarations in Implementations and Match Exprs Use Declarations in More Places Apr 18, 2017
@burdges
Copy link

burdges commented Apr 18, 2017

Just fyi, I think mixing ; vs , in match, struct { }, and enum sounds okay. In particular, the trait fields RFC already considered alternatives for introducing field to traits and impls and they prefered on breaking any syntactic convention against mixing ; and ,. If anything, your syntax is easier since the ; usually comes first.

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the RFC. label Apr 19, 2017
@Mark-Simulacrum
Copy link
Member

I would say that we should, at least for the time being, keep to just impl blocks. I don't think use in match or elsewhere is all that useful, since you can usually just put it in a block around the match (or the function which the match is in); constraining this RFC to just impl blocks seems like a good start to this.

However, I don't feel strongly about this--if we can get it with everything, then perhaps that's better. I do have some concerns about this RFC running into the same problems the else match RFC ran into (scoping issues, primarily; what do we include? What makes sense? etc.). These scoping issues are certainly not as major a problem for this RFC I think since it's changes are more minimal in terms of where they can go.

@est31
Copy link
Member

est31 commented Apr 20, 2017

@Mark-Simulacrum its very useful in match, to not having to write the enum name in every case.

@kennytm
Copy link
Member

kennytm commented Apr 20, 2017

For use-in-match, regarding the drawback,

For use in match, this means that we can allow things other than pattern => expr that are separated by semicolons and not commas.

the match expression grammar can just be changed to specifically only allowing use and =>:

"match" expr "{" (attr* "use" paths ";")* (pat "=>" (expr "," | block))+ "}"

There is no need to allow more things inside the match.

@burdges
Copy link

burdges commented Apr 20, 2017

It's interesting if you find a complication with adding use to any of impl, match, enum, struct, or trait.

@Mark-Simulacrum
Copy link
Member

I agree that there's likely no problem with adding use to any of these statements. I think it might almost be a subconscious reaction to something. I'm generally all for adding use to more places, especially logical ones such as these.

@ghost
Copy link

ghost commented Apr 20, 2017

Perhaps use statements should only be allowed as the first thing inside the match block, before any of the arms. That would solve the code clarity problem. Any use statements come first, then list the comma-separated stuff. Same with structs and enums, actually.

This also logically deals with the problem of mixing semicolon-terminated statements with comma-separated things. You could logically say that the comma-separated list hasn't started until after all the semicolon-terminated statements.

@kornelski
Copy link
Contributor

It'd be nice if it was completely generalized to allow use as the first thing after any {, whether it's impl, match, or struct.

@kennytm
Copy link
Member

kennytm commented Apr 20, 2017

@pornel use std::io::{use std::fs::File; Read, Write}; (I kid, I kid 😄)

But I wonder if it makes sense for struct literal:

let foo = Foo {    // <--
    use std::cmp::Ordering::*;
    a: Greater,
    b: Less,
}

(and if yes the question becomes "why not tuples as well")

@kornelski
Copy link
Contributor

I mean from perspective of learning the language, it's easier to remember "all block-like things can start with use" than "use can be used here and there".

@aturon aturon self-assigned this Apr 27, 2017
@aturon
Copy link
Member

aturon commented Apr 29, 2017

There's definitely a sentiment on the lang team that over time, the body of impl blocks and modules should converge in what they support.

The reason to single out impl blocks is that they already contain a (restricted subset) of items -- which is to say, the things you can write in an impl block are things you could also write at the module level. But not vice versa. Adding use declarations helps further bridge the gap. And of course it addresses a practical concern around fine-grained scoping control, as well as learnability.

The case for match is, I think, much less strong, given that match arms are quite distinct from the item definitions that appear in modules (or other places you can write use today). There's also the fact that match arms have semantics that depend on ordering, making use a quite awkward fit.

So, 👍 for use in impl blocks as a starting point.

@nrc
Copy link
Member

nrc commented Apr 30, 2017

I'm in favour of allowing this in impls, but not inside match (I'd also be OK with allow it inside traits (and given that default methods exist, we probably should, if we allow in impls), but not structs or enums).

@clarfonthey
Copy link
Contributor

clarfonthey commented May 2, 2017

I agree with adding this to traits as well; because it's not as syntactically weird I think that we should at least group impl and trait together and do both or neither.

One potential conflict though is a potential conflict with #1406.

Remove `use` in `match`. Should be as own RFC, and not willing to defend it personally.
Likewise, removed parts in the how we teach and drawbacks, making those sections less
"if"fy.

Added `trait`, as that's analogous to `impl` and I feel stupid for not doing that initially.

Added grammar changes. Not sure if they're the right changes, but at least they exist.
@Havvy
Copy link
Contributor Author

Havvy commented May 2, 2017

Alright, going by discussion, I removed the use in match part. That can become its own RFC later (or even right now) if somebody wants to champion it. I personally don't. Getting a conservative change in quickly is better than waiting forever to argue about where else it should be allowed.

I also added a grammar section. Somebody who understands the grammar file better than I, please do so.

And likewise, since traits are analogous to implementations, I added traits to it. No examples though, as I couldn't really think of a good one quickly. Not entirely sure it's needed either, as I think if you know how to use use in blocks and modules, you already will intuit the usage of use in traits and implementations.

@Havvy
Copy link
Contributor Author

Havvy commented May 2, 2017

@clarcharr I don't think it's a conflict. The syntax they propose would give a completely new meaning to use, which would add confusion. So I think that if that RFC passes, it will be via some other syntax.

@clarfonthey
Copy link
Contributor

@Havvy fair! Although I think that it's worth mentioning in the drawbacks section that there might be a conflict between these two RFCs.

@clarfonthey
Copy link
Contributor

Also @Havvy currently the rendered link is broken.

@Havvy
Copy link
Contributor Author

Havvy commented May 3, 2017

Render link fixed.

I don't consider it worth being a drawback worth writing because it's the same as any other RFC proposing syntax where it blocks off any other RFC from using the same syntax. Should the other RFC also add as a drawback that it conflicts with this one?

@aturon
Copy link
Member

aturon commented May 7, 2017

Thanks @Havvy!

Lang team: the proposal here is to allow use declarations within trait and impl blocks. We've long thought that such blocks should converge with modules, in terms of the items allowed, and this seems like a fine step in that direction, with some ergonomic and readability benefits.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 7, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@petrochenkov
Copy link
Contributor

petrochenkov commented May 7, 2017

SUPERSEDED BY #1976 (comment)


I have some concerns.

use in traits and impls has one more pretty intuitive interpretation described in RFC 1150.
In this interpretation aliases defined with use are visible outside of traits and impls they are defined in. I'm pretty sure that people will expect things from RFC 1150 to work after this RFC is implemented. Example:

struct S;

impl S {
    fn f(&self) {}
    use f as g; // or Self::f as g or something
}

S.g(); // Why doesn't it work?!

This can be made to work with use in traits, traits are "modules" from name resolution point of view and in theory you could reexport any items from them, including non-associated items. Example:

struct S;

trait Tr {
    use S as Z;    
}

let s: S = Tr::Z; // OK

This may be a bit surprising, but I think it's better than introducing one more special case into the language, like this RFC does.
(BTW, traits as "modules" are a bit limited right now, e.g. you can't do

use Default::default;

let x = default();

, but this is legitimately useful, improves consistency and can be easily fixed, so I expect this to be fixed.)

The problem with S.g(); from the first example is that the expected behavior can't be implemented without either rewriting half of the compiler, or/and twisting the meaning of use and breaking the clean separation between "name based" and "type based" parts of the language.
The @aturon's comment ("the things you can write in an impl block are things you could also write at the module level. But not vice versa. Adding use declarations helps further bridge the gap.") is a bit frightening if it implies the intent to break this separation.


In the end, I can live with use in traits if it naturally fits into the current system and works as described above ("trait is a module").
I also can live with use in impls, but it will be a confusing feature and basis for hardly fulfillable feature requests.
(I don't think I'll use this new feature myself due to preference of the "all uses at the top of the file" style.)

@nikomatsakis
Copy link
Contributor

I am inclined to agree with @petrochenkov's objections to the current RFC. It seems to me that, way back when, when we decided to make pub use and use share a keyword (they used to be distinct concepts, with use being import, and I forget what pub use was, link or something), we actually sort of conflated two distinct things in the use keyword: local aliases and links. This is fairly harmless at the module level, because of our approach to privacy, though I think it does (at present, anyway) lead to some confusing error messages (e.g., when you do foo::bar but foo only has a use of bar, you get something talking about privacy, rather than saying "no item bar defined in foo). But as @petrochenkov points out, this overlap becomes a problem at levels other than the module.

@petrochenkov's use clauses seem like an interesting way to separate those two items back out again.

@Havvy
Copy link
Contributor Author

Havvy commented May 27, 2017

Right. The question then becomes, is it worth adding the complexity of use clauses to the language right now? I want to say "no" for now, personally. In any case, it'd effectively be its own RFC, so I'm thinking of just closing this one.

@phaylon
Copy link

phaylon commented May 27, 2017

In the past there were discussions about having let x = y in <expr> and let x = y in { ... } constructs. I'm wondering if the possibility to extend that to general scoping has come up, to also allow use x in <expr> and use x in { ... }? The latter would apply to impls, structs, and so on as well. It also has the advantage that I find it suggests that re-export via pub use doesn't make sense for in, and keeps other more specific use positions free for future enhancements.

@Kimundi
Copy link
Member

Kimundi commented May 27, 2017

I also fully agree with @petrochenkov's assessment here. use items bring names into scope and defines links to other items, independent of pub, and we should not introduce different semantics for it in places where we already have established semantics for other kinds of items.

use clauses as a separate syntactic mechanism that only causes the bringing-into-scope-effect seems like a clean solution for this use case.

@phaylon: What I find problematic about that syntax is that it defines aliases as a prefix of the item syntax, so it might be more confusing to read if you bring a lot of items into scope, since it takes the code a while to "get to the point" of what these aliases are actually for.

@phaylon
Copy link

phaylon commented May 27, 2017

@Kimundi Do you mean having multiple imports? I think having multiple import targets would be a nice solution, e.g.:

use toml,
    serde,
    something::internal::{ with_functions },
in {
    struct MyStruct {
        ...
    }

    impl MyStruct {
        ...
    }
}

Or am I misunderstanding?

@Kimundi
Copy link
Member

Kimundi commented May 27, 2017

@phaylon: Basically I mean, that if you have something like this:

use foo::Bar, ... in struct X { ... }

Then you have to skip over the use foo::Bar ... while reading the code to see that it applies to a struct.

A syntax like

struct X where use foo::Bar, ... { ... }

has the advantage that the use reads more as an internal aspect of the definition, which it is.

@crumblingstatue
Copy link

My opinion:

There are many RFCs with more pressing motivation than this that were postponed.

Use declarations at the module and block level suffice for the vast majority of use cases, and this change seems quite controversial, so I propose to postpone this.

@phaylon
Copy link

phaylon commented May 27, 2017

@Kimundi Ah, that is true. For me that would be outweighed by the fact that I can use it to apply imports to multiple items, and that the construct is expression compatible as-is.

@Kimundi
Copy link
Member

Kimundi commented May 28, 2017

@phaylon: True. Actually I think the block form (use ... in { ... }) on its own would be more acceptable, as proper indentation of the items/expressions in the block would make them as readable as regular use lines at the beginning of a module.

@aturon
Copy link
Member

aturon commented May 31, 2017

@crumblingstatue

There are many RFCs with more pressing motivation than this that were postponed.

Use declarations at the module and block level suffice for the vast majority of use cases, and this change seems quite controversial, so I propose to postpone this.

That's a pretty compelling argument; I think @petrochenkov's counter-proposal is worth keeping in mind, but we should turn our ergonomic attention elsewhere for now.

@rfcbot fcp postpone

@aturon
Copy link
Member

aturon commented Jun 2, 2017

@rfcbot fcp cancel

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 2, 2017

@aturon proposal cancelled.

@aturon
Copy link
Member

aturon commented Jun 2, 2017

@rfcbot fcp postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 2, 2017

Team member @aturon has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@withoutboats withoutboats added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jun 21, 2017
@qnighy
Copy link

qnighy commented Jun 22, 2017

@nikomatsakis I was interested by your comment in pub use/use merger and had a look at old versions. Only thing I discovered was that use was import and pub use was import with export like here. So my understanding is these two concepts were already expressed in the same import system, back in the version 0.1 (the keyword changed in 0.4). Perhaps the merger occurred before 0.1?

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 6, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

1 similar comment
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 6, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Jul 6, 2017
@nikomatsakis
Copy link
Contributor

@qnighy

Only thing I discovered was that use was import and pub use was import with export like here. So my understanding is these two concepts were already expressed in the same import system, back in the version 0.1 (the keyword changed in 0.4). Perhaps the merger occurred before 0.1?

That was the "non-unified" version I was talking about. In the olden days, export foo was how you, well, exported something from a module. It was not necessarily something you imported, iirc.

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 16, 2017

The final comment period is now complete.

@Havvy
Copy link
Contributor Author

Havvy commented Jul 19, 2017

Given the lack of comments for a month, and the FCP being complete w/postpone, I'm going to close this.

@Havvy Havvy closed this Jul 19, 2017
@Havvy Havvy deleted the use-in-impl-and-match branch September 11, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet