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.

Sign up for GitHub

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

Jump to bottom

Methods that can fail (panic) should be marked as unsafe #17650

Closed
sergei-kucher opened this issue Sep 30, 2014 · 3 comments
Closed

Methods that can fail (panic) should be marked as unsafe #17650

sergei-kucher opened this issue Sep 30, 2014 · 3 comments

Comments

@sergei-kucher
Copy link

Some of core methods using fail! macros. Like expect and unwrap of core::option::Option and core::result::Result.

I found two primary things about this:

Seems like some of fail (panic) methods will be preserved (like expect for a Option<V>). But using of such methods is highly error-prone.

To prevent mass usage of them I suggest to mark such methods as unsafe.

For example:

unsafe fn expect(self, msg: &str) -> T
@sergei-kucher sergei-kucher reopened this Sep 30, 2014
@sinistersnare
Copy link
Contributor

unsafe is used for operations that are memory unsafe. marking (supposed) code smells as unsafe would only pollute when or when not to use it. notice memory unsafe is a very clear definition, unsafe does not mean this code might be broken, per se. (as i understand this)

@thestinger
Copy link
Contributor

unsafe is for drawing a boundary between memory unsafe and memory safe code

http://doc.rust-lang.org/reference.html#unsafety

It's not used to discourage the usage of safe APIs as that would break down the boundary and make it harder to identify memory safety issues. Usage of unwrap and expect is common in correct code, because there are invariants that are not encoded into the type system. Changing the meaning of unsafe would be a fundamental change to the design of the language / libraries so it would need to go through the RFC process (but it wouldn't be accepted).

@thestinger
Copy link
Contributor

(now at rust-lang/rfcs#340)

lnicola pushed a commit to lnicola/rust that referenced this issue Jul 28, 2024
@bors
Auto merge of rust-lang#17650 - ObsidianMinor:fix/17645, r=Veykril
449d4c4 
Fix path resolution for child mods of those expanded by `include!`

Child modules wouldn't use the correct candidate paths due to a branch that doesn't seem to be doing what it's intended to do. Removing the branch fixes the problem and all existing test cases pass.

Having no knowledge of how any of this works, I believe this fixes rust-lang#17645. Using another test that writes the included mod directly into `lib.rs` instead, I found the difference can be traced to the candidate files we use to look up mods. A separate branch for if the file comes from an `include!` macro doesn't take into account the original mod we're contained within:

```rust
None if file_id.macro_file().map_or(false, |it| it.is_include_macro(db.upcast())) => {
    candidate_files.push(format!("{}.rs", name.display(db.upcast())));
    candidate_files.push(format!("{}/mod.rs", name.display(db.upcast())));
}
```

I'm not sure why this branch exists. Tracing the branch back takes us to 3bb9efb but it doesn't say *why* the branch was added. The test case that was added in this commit passes with the branch removed, so I think it's just superfluous at this point.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 1, 2024
@bors
Auto merge of rust-lang#17650 - ObsidianMinor:fix/17645, r=Veykril
d9b704c 
Fix path resolution for child mods of those expanded by `include!`

Child modules wouldn't use the correct candidate paths due to a branch that doesn't seem to be doing what it's intended to do. Removing the branch fixes the problem and all existing test cases pass.

Having no knowledge of how any of this works, I believe this fixes rust-lang#17645. Using another test that writes the included mod directly into `lib.rs` instead, I found the difference can be traced to the candidate files we use to look up mods. A separate branch for if the file comes from an `include!` macro doesn't take into account the original mod we're contained within:

```rust
None if file_id.macro_file().map_or(false, |it| it.is_include_macro(db.upcast())) => {
    candidate_files.push(format!("{}.rs", name.display(db.upcast())));
    candidate_files.push(format!("{}/mod.rs", name.display(db.upcast())));
}
```

I'm not sure why this branch exists. Tracing the branch back takes us to 3bb9efb but it doesn't say *why* the branch was added. The test case that was added in this commit passes with the branch removed, so I think it's just superfluous at this point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@thestinger @sinistersnare @sergei-kucher