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

Adding unsafe modules and unsafe blocks outside functions. #2148

Closed
wants to merge 2 commits into from

Conversation

porky11
Copy link

@porky11 porky11 commented Sep 12, 2017

This allows unsafe for blocks outside of functions and for modules.
It will enable faster prototyping and more compact writing of unsafe libs.

Edit: Rendered

@Centril
Copy link
Contributor

Centril commented Sep 13, 2017

I like the general idea behind this RFC; but I think it needs to be a lot more detailed than it currently is.

This should fit nicely in the ergonomics initiative to remove paper cuts.

@egilburg
Copy link

egilburg commented Sep 13, 2017

If this is something usually resorted to as temporary/prototype mechanism, would #[unsafe] attribute be more suited than a keyword? Marking a module as #[unsafe] would mark all functions inside as unsafe, etc.

This would also avoid massive indent/unindent of mod body if the attribute is added or removed.

@bbatha
Copy link

bbatha commented Sep 13, 2017

If this is something usually resorted to as temporary/prototype mechanism, would #[unsafe] attribute be more suited than a keyword? Marking a module as #[unsafe] would mark all functions inside as unsafe, etc.

You'd want this if your using the entire file as a module. It seems weird and non-local to have the unsafe declaration be in the mod statement outside the file, e.g.

lib.rs:

unsafe mod foo;

foo.rs

type Foo;
type Bar
fn baz() {}

vs

foo.rs

#![unsafe]
type Foo;
type Bar
fn baz() {}

In fact, if you only get to use unsafe at the declaration site you could accidently make a module safe if you declare the module in two places if say your crate has a binary and a library in it.

lib.rs

unsafe mod foo;

bin/foo.rs

mod foo; // oops

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the RFC. label Sep 14, 2017
But unsafe modules have an additional property.
Using such a module is also unsafe:
```
unsafe use test;
Copy link
Member

Choose a reason for hiding this comment

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

I find this surprising, because use doesn't need unsafe to import other unsafe things:

mod foo { pub unsafe fn bar() {} }
use foo::bar; // no unsafe

Copy link
Contributor

Choose a reason for hiding this comment

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

Still perhaps understandable...? With normal modules the entire module isn't necessarily for unsafe stuff. And unsafe use can be good for greppability. Then again, you still have to be in "unsafe mode" to use the functions inside the module, and you can grow for that.

Copy link
Member

@scottmcm scottmcm Sep 15, 2017

Choose a reason for hiding this comment

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

It means that "this could just be implemented by some syntactic transformations" isn't true, though. It's a new concept that users need to know, but I'm not convinced there's value in it.

Copy link
Author

Choose a reason for hiding this comment

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

I also wasn't sure if unsafe use is useful. But you don't even need to use unsafe functions from safe modules, and just use them at other positions. Maybe just importing things from inside the module as unsafe would make sence? But I think I should remove unsafe use from the RFC entirely. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say just remove it, especially since it opens a bunch of questions about what paths involving the module would require. For example:

unsafe mod foo {
    pub struct Bar;
}
use foo::Bar; // do I need `unsafe use` here?

(Thanks to Havvy for inspiration that led to this example.)

@porky11
Copy link
Author

porky11 commented Sep 17, 2017

@Centril
At least unsafe blocks outside of
I thought your last example would just declare a new submodule functions would fit ergonomics, I think.

@egilburg
interesting idea.
But wouldn't be as intuitive.

@bbatha
I thought your last example would just declare a new submodule (foo::foo) which is then unsafe, too, because it's declared inside a module.

add alternative option `#[unsafe]`
@scottmcm
Copy link
Member

Hmm, one can already define functions inside unsafe blocks, and they're not unsafe, so this would make inner functions behave differently. Silly demo:

fn foo() {
    let x: fn();
    unsafe {
        fn bar() {}
        x = bar;
    }
    x()
}

More generally, this RFC would mean that unsafe blocks would sometimes mean "I'm providing a safe interface over unsafe things" and sometimes "I'm defining a bunch of unsafe things" which are very different things, which makes me nervous.

@burdges
Copy link

burdges commented Sep 18, 2017

We want unsafety to be explicit and rare, so we should require that every unsafe item be marked unsafe.

An unsafe trait is a just trait where writing the impl requires upholding invariants, so you must write unsafe impl to make this promise. All methods of an unsafe trait are themselves safe unless explicitly marked as unsafe, and several core traits like FixedSizeArray and TrustedRandomAccess depend on this, so this RFC would either be a nasty breaking change or create inconsistencies.

I think by analogy an unsafe module would be a module that requires an unsafe use to access, which may not make sense, or maybe it can only be used from unsafe code. It's items should all be safe unless explicitly marked otherwise of course.

@Havvy
Copy link
Contributor

Havvy commented Sep 18, 2017

A consistent treatment of this would mean that mod items inside an unsafe mod would also have to be an unsafe mod, so to figure out whether a file is unsafe, one must look at all its ancestors to figure out whether they are an unsafe mod. Having the safety of an item not be locally knowable is bad. The alternative of not letting it affect mods as an ad-hoc rule means another ad-hoc rule in the compiler, which I'm against as well.

In general, I'm against this idea because unsafe on each item is something we want to make explicit for grepping. Trying to make it any more implicit muddies this use-case.

@porky11
Copy link
Author

porky11 commented Sep 27, 2017

I understand the problem with fn in unsafe blocks.
What about #[unsafe] or an unsafe compiler option (at least for prototyping, see alternatives)

@main--
Copy link

main-- commented Sep 28, 2017

I feel like the biggest issue here is motivation. I don't even see any problem to begin with - when is unsafe fn vs fn ever a significant inconvenience?

@nikomatsakis
Copy link
Contributor

@rfcbot fcp postpone

I'd rather not change anything about the syntax of unsafe until the unsafe code guidelines work makes more progress. We may want to tinker with the semantics here in other ways, and I think it's better to leave things as they are until that time. Therefore, I move to postpone this RFC to be revisited in the future. (Thanks to @porky11 for the suggestion in any case. ❤️ )

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 26, 2018

Team member @nikomatsakis 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.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 26, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 31, 2018

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

1 similar comment
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 31, 2018

🔔 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 Jan 31, 2018
@burdges
Copy link

burdges commented Feb 2, 2018

Appears even sorting out unsafe fn is a mess : #2316 (comment)

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 10, 2018

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Feb 14, 2018

Closing as postponed. Thanks for the RFC, @porky11!

@aturon aturon closed this Feb 14, 2018
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.