Skip to content

Fix nightly warnings#3421

Merged
stevenengler merged 3 commits intoshadow:mainfrom
stevenengler:nightly-fixes
Oct 8, 2024
Merged

Fix nightly warnings#3421
stevenengler merged 3 commits intoshadow:mainfrom
stevenengler:nightly-fixes

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Oct 6, 2024

Fixes elided_named_lifetimes, static_mut_refs, and non_local_definitions warnings.

Apparently they are considering making non-local impl definitions a hard error in the future (so we won't be able to allow(non_local_definitions) anymore), which would be unfortunate since we wouldn't be able to have our syscall logger macros at the beginning of the syscall handler functions anymore. But this wouldn't take effect until 2028, so I'm happy to ignore this.

For static_mut_refs we can make it a little nicer using raw references when they release in rust 1.82 in ~10 days.

@stevenengler stevenengler self-assigned this Oct 6, 2024
@github-actions github-actions bot added Component: Libraries Support functions like LD_PRELOAD and logging Component: Testing Unit and integration tests and frameworks Component: Main Composing the core Shadow executable labels Oct 6, 2024
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

Their plan to make non-local impl defs a hard error is troubling. I know we have a couple of years but still 🤯

@stevenengler
Copy link
Copy Markdown
Contributor Author

Their plan to make non-local impl defs a hard error is troubling. I know we have a couple of years but still 🤯

Yeah I don't think I'm a fan of the change; the way we use it is a bit of a hack, but there aren't any nice alternatives. A good thing is that it would only be in the 2027 edition, so if we didn't update the edition number in the Cargo.toml, it should still theoretically build fine past that date.

Also it's only the syscall logger macros, so if we needed to we could easily drop the syscall logging from shadow.

```text
warning: elided lifetime has a name
  --> lib/gml-parser/src/parser.rs:45:65
   |
45 | pub fn key<'a, E: GmlParseError<'a>>(input: &'a str) -> IResult<&str, &str, E> {
   |            -- lifetime `'a` declared here                       ^ this elided lifetime gets resolved as `'a`
   |
   = note: `#[warn(elided_named_lifetimes)]` on by default
```
```text
warning: creating a mutable reference to mutable static is discouraged
    --> test/signal/test_signals.rs:1164:22
     |
1164 |             unsafe { DO_SETCONTEXT_RES.take() },
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^ mutable reference to mutable static
     |
     = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html>
     = note: mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives
     = note: `#[warn(static_mut_refs)]` on by default
```
```text
warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
   --> main/utility/macros.rs:148:13
    |
147 |           const $const_name : () = {
    |           ---------------------- move the `impl` block outside of this associated constant `_syscall_logger_waitid`
148 |               impl crate::utility::macros::SyscallLogger {
    |               ^^^^^-------------------------------------
    |                    |
    |                    `SyscallLogger` is not local
    |
   ::: main/host/syscall/handler/wait.rs:232:5
    |
232 | /     log_syscall!(
233 | |         waitid,
234 | |         /* rv */ kernel_pid_t,
235 | |         /* which */ c_int,
...   |
239 | |         /* uru */ *const std::ffi::c_void,
240 | |     );
    | |_____- in this macro invocation
    |
    = note: the macro `log_syscall` defines the non-local `impl`, and may need to be changed
    = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
    = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <rust-lang/rust#120363>
    = note: this warning originates in the macro `log_syscall` (in Nightly builds, run with -Z macro-backtrace for more info)
```
@stevenengler stevenengler enabled auto-merge October 8, 2024 22:48
@stevenengler stevenengler merged commit 9bde9ad into shadow:main Oct 8, 2024
@stevenengler stevenengler deleted the nightly-fixes branch October 8, 2024 23:46
@sporksmith
Copy link
Copy Markdown
Contributor

Their plan to make non-local impl defs a hard error is troubling. I know we have a couple of years but still 🤯

Yeah I don't think I'm a fan of the change; the way we use it is a bit of a hack, but there aren't any nice alternatives. A good thing is that it would only be in the 2027 edition, so if we didn't update the edition number in the Cargo.toml, it should still theoretically build fine past that date.

Also it's only the syscall logger macros, so if we needed to we could easily drop the syscall logging from shadow.

+1 that would be a bummer. I remember I was a bit skeptical about adding it in the first place, but it's been SUPER nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Libraries Support functions like LD_PRELOAD and logging Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants