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

Stabilize #[unix_sigpipe = "sig_dfl"] on fn main() #120832

Closed

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Feb 9, 2024

I would like to propose that we stabilize #[unix_sigpipe = "sig_dfl"].
This PR does that.

By my estimation, merging this PR will close #46016.

Tracking issue is #97889.

Stabilization report

The #[unix_sigpipe = "sig_dfl"] attribute has been available in nightly since nightly-2022-09-04 (17 months).

It is being used by rustc and rustdoc themselves, and also by third party code (1, 2).

I have monitored the keyword sigpipe on rust-lang/rust during all this time, and no problems have been reported regarding the attribute. To the contrary. It's infrastructure was used to implement a bugfix in a backwards compatible way.

Summary and examples

Everything you need to know about the stabilized attribute should be documented in the reference. PR. Rendered.

Test cases

https://github.com/rust-lang/rust/tree/master/tests/ui/attributes/unix_sigpipe

Edge cases

None that I am aware of.

Note that using #[unix_sigpipe = "sig_dfl"] and receiving a SIGPIPE means destructors will not run since the process will be immediately killed. I documented this in The Reference.

Unresolved questions

None. See tracking issue.

Worth noting

This PR does NOT stabilize any other variant of the attribute, namely #[unix_sigpipe = "sig_ign"] and #[unix_sigpipe = "inherit"].

There are many reasons for this:

  • Avoids choice paralysis between leaving SIGPIPE be the default (SIG_IGN) and explicitly setting it to SIG_IGN with #[unix_sigpipe = "sig_ign"]
  • I have found no one using #[unix_sigpipe = "sig_ign"] nor #[unix_sigpipe = "inherit"] in the wild, so they are much less tested, and there seems to be no desire for them.
  • Stabilizing something means committing to its existence for all eternity, so we should not stabilize attributes that are not absolutely needed.
  • It leaves the door open to change fn lang_start() sigpipe arg from u8 to bool ("not set" or "set to sig_dfl"), enabling better lto. Only relevant if we end up deciding not to change the default, and not adding #[unix_sigpipe = "inherit"].
  • These variants have unresolved open questions. See tracking issue.

Potential future work

There is a discussion about changing the default SIGPIPE handler. See here. Changing the default would likely require us to also stabilize #[unix_sigpipe = "sig_ign"]. But doing that and discussing changing the default is out of scope of this PR.

Left to do

  • Update the unstable book page of unix_sigpipe if we merge this.

FCP

Since we can't close the tracking issue entirely yet, my hope is that we can do an FCP on this PR rather than on the tracking issue.

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 9, 2024
@rust-log-analyzer

This comment has been minimized.

@Noratrieb Noratrieb added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 9, 2024
@Enselic Enselic force-pushed the stabilize-unix_sigpipe-sig_dfl branch from 4be319b to 775ee4e Compare February 9, 2024 09:46
@Enselic Enselic added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 9, 2024
@Enselic Enselic mentioned this pull request Feb 11, 2024
23 tasks
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me, will wait for the t-lang nomination.

@joshtriplett
Copy link
Member

joshtriplett commented Feb 14, 2024

I do think we should stabilize "inherit". In particular, if we consider changing the default in a future edition, I'd propose changing the default to "inherit", not to "sig_dfl", so that we don't make the syscall at all.

@kornelski
Copy link
Contributor

I've mentioned in the forum that I'm missing an option to ignore both the signal and panic-causing errors in println.

If this attribute wasn't so transparent with low-level unix jargon, maybe it could be used to set the third strategy?

@Enselic
Copy link
Member Author

Enselic commented Feb 16, 2024

I do think we should stabilize "inherit". In particular, if we consider changing the default in a future edition, I'd propose changing the default to "inherit", not to "sig_dfl", so that we don't make the syscall at all.

I don't think we should stabilize #[unix_sigpipe = "inherit"], because if we change the default in a future edition to be to not touch SIGPIPE at all we would have

fn main() SIGPIPE
fn main() SIGPIPE not touched
#[unix_sigpipe = "sig_ign"]
fn main()
SIGPIPE set to SIG_IGN
#[unix_sigpipe = "sig_dfl"]
fn main()
SIGPIPE set to SIG_DFL

Maybe we don't even need #[unix_sigpipe = "sig_dfl"] at all in that case.

So maybe we need to make a decision on #62569 after all, before we can stabilize anything. Perhaps summarizing #62569 into an RFC is the best way forward here. I think it is, and I hope to get around to it in a not too distant future.

If this attribute wasn't so transparent with low-level unix jargon, maybe it could be used to set the third strategy?

I replied to your post in the forum. In short: I don't think #[unix_sigpipe = "..."] will be useful for this.

@joshtriplett
Copy link
Member

@Enselic What's the harm in stabilizing inherit even if it subsequently becomes the default?

We allow people to write repr(Rust), or pub(self), even though those are the defaults.

@bors
Copy link
Contributor

bors commented Feb 16, 2024

☔ The latest upstream changes (presumably #120881) made this pull request unmergeable. Please resolve the merge conflicts.

@Enselic
Copy link
Member Author

Enselic commented Feb 17, 2024

@joshtriplett It is possible I am overestimating the harm of stabilizing an attribute that might not be needed long term. 10 years from now I just think it will be annoying to have to maintain an attribute variant that is not needed. It unnecessarily restricts us.

If we can find a way to turn sigpipe: u8 of fn lang_start() into a sigpipe: bool, we will achieve better lto. Meaning that the code for the call to libc::signal can more easily be removed entirely.

I also think that "no attribute used" is a good way to convey "no special code runs".

If we want to stabilize #[unix_sigpipe = "inherit"], I can't help but want to bikeshed the name a bit more. Looking at man 7 signal we can see that "inherit" happens during fork():

A child created via fork(2) inherits a copy of its parent's signal dispositions.

so I don't think "inherit" is a slam-dunk name to use in our case, because the "inherit" mechanism has already been applied when the Rust lang_start() code runs.

Maybe #[unix_sigpipe = "unchanged"] is a better name?

@kornelski
Copy link
Contributor

Maybe change the default on edition boundary, and only support a Boolean attribute that restores the old behavior for cargo fix to use?

@jstarks
Copy link

jstarks commented Feb 18, 2024

If we stabilize inherit and skip stabilizing dfl, then people who want dfl can explicitly make the call in main themselves. And then you can remove the dfl option altogether and get your lto win.

If we don’t stabilize inherit, then people who want standard UNIX behavior have no way to get it, other than wait for an unspecified new edition, I guess? Which in reality is another 3 years away, at least?

@joshtriplett
Copy link
Member

Discussed in today's @rust-lang/libs meeting: libs does not consider ourselves a blocker for this stabilization.

@joshtriplett joshtriplett removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 21, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? `@davidtwco` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? ``@davidtwco`` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2024
…r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? ```@davidtwco``` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 28, 2024
…r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? `@davidtwco` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 28, 2024
…r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? ``@davidtwco`` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
Rollup merge of rust-lang#121527 - Enselic:unix_sigpipe-tests-fixes, r=davidtwco

unix_sigpipe: Simple fixes and improvements in tests

In rust-lang#120832 I included 5 preparatory commits.

It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way.

r? ``@davidtwco`` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then).

For the convenience of my reviewer, here are the full commit messages of the commits:
<details>
<summary>Click to expand</summary>

```
commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes)
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 07:57:27 2024 +0100

    tests: Add unix_sigpipe-different-duplicates.rs test variant

    To make sure that

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "y"]

    behaves like

        #[unix_sigpipe = "x"]
        #[unix_sigpipe = "x"]

commit d14f158
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 08:47:47 2024 +0100

    tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs

    The only difference between the files is the presence/absence of

        #![feature(unix_sigpipe)]

    attribute. Avoid duplication by using revisions instead.

commit a1cb3db
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 06:44:56 2024 +0100

    tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity

    The test is for the "bare" variant of the attribute that looks like this:

        #[unix_sigpipe]

    which is not allowed, because it must look like this:

        #[unix_sigpipe = "sig_ign"]

commit e060274
Author: Martin Nordholts <martin.nordholts@codetale.se>
Date:   Fri Feb 9 05:48:24 2024 +0100

    tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs

    There is no error expected. It's simply the "regular" test for sig_ign.
    So rename it.
```

</details>

Tracking issue: rust-lang#97889
Enselic added 2 commits March 9, 2024 11:33
That is the variant that has the highest potential to be stabilized, and
we want to test it as much as possible.
The attribute is used like this:

    #[unix_sigpipe = "sig_dfl"]
    fn main() {
        // ...
    }

When used, `SIGPIPE` will be set to `SIG_DFL` before `fn main()` is
invoked, rather than being set to `SIG_IGN` which is the default.

This commit does NOT stabilize `#[unix_sigpipe = "sig_ign"]` or
`#[unix_sigpipe = "inherit"]`.

See the PR for more info.
@Enselic Enselic force-pushed the stabilize-unix_sigpipe-sig_dfl branch from 775ee4e to b6d7e9d Compare March 9, 2024 14:44
@Enselic
Copy link
Member Author

Enselic commented Mar 9, 2024

Rebased to resolve conflicts. Current status:

If we decide not to change the default SIGPIPE disposition which is my recommendation, there are no blockers to merge this PR after an FCP so that #[unix_sigpipe = "sig_dfl"] is stabilized.

If we want to change the default SIGPIPE disposition to "inherit" over an edition boundary, we probably still want to stabilize #[unix_sigpipe = "sig_dfl"], but it is less of a slam dunk because in practice SIG_DFL will be the default.

We can't stabilize #[unix_sigpipe = "sig_ign"] yet because currently child processes get SIG_IGN, but arguably they should get SIG_DFL since that is what most programs assume, and we explicitly made it that way before. Note that we can implement this without running code right before exec, see #121578 for the trick. This open question is mentioned in the tracking issue now.

We can't stabilize #[unix_sigpipe = "inherit"] yet because we might want to rename it to e.g. #[unix_sigpipe = "unchanged"]. This open question is mentioned in the tracking issue now.

@bstrie
Copy link
Contributor

bstrie commented Mar 9, 2024

On behalf of the Edition 2024 Working Group, even though our soft deadline for accepting editions changes has passed, we're still willing to entertain changing the SIGPIPE default in the 2024 edition, but the hard deadline for the edition cutoff is the end of May, so please make a decision promptly. :)

@hypatian
Copy link

hypatian commented Mar 9, 2024

As someone who writes a lot of software that works by connecting things up with pipes: I strongly encourage at least getting #[unix_sigpipe = "sig_dfl"] out of nightly.

After reading around in 62569, I understand better now some of the concerns around changing the Rust signal-handling default behavior. While as a UNIX-style command-line tool dev I'd much rather not have been surprised by this difference from other development environments on UNIX, it was not that hard to find out where the surprise was coming from and to resolve it.

Except, that is, for the fact that it's locked me into using nightly for basically all of my Rust development ever. That's really my only beef: it's been years, and I'm still using nightly just for this.

So change the default for edition 2024 or don't... but please, please, at least make it possible for developers of UNIX pipe-based tools to use stable instead of having to take on nightly just to get back to their operating system's default behavior.

@Amanieu
Copy link
Member

Amanieu commented Mar 10, 2024

As a completely separate concern from what to do about the default behavior, I have some concerns about the attribute syntax itself.

Currently, the attribute describes the syscall that is being performed (setting the signal disposition for SIGPIPE), but this is a very low-level API that may be confusing to people not familiar with the weeds of UNIX pipes. Additionally, #[unix_sigpipe = "sig_ign"] would be misleading if #121578 lands since we won't actually be using SIG_IGN at all in that case.

I believe that the attribute should instead describe what it is actually achieving: in this case, changing the behavior of the program when a pipe is broken. Proposed syntax: #[on_broken_pipe = "..."] or #[unix_on_broken_pipe = "..."].

Possible values for the attribute would be:

  • "inherit" to inherit the behavior from the parent process.
  • "kill" to kill the process with SIGPIPE.
  • "error" to have writes return an error (EPIPE).

@bors
Copy link
Contributor

bors commented Mar 11, 2024

☔ The latest upstream changes (presumably #122312) made this pull request unmergeable. Please resolve the merge conflicts.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 11, 2024
…idtwco

unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests

The `sig_dfl` variant of the attribute is the most likely variant to be stabilized first, and thus to become the "most allowed" variant of the attribute. Because of this, it is the most appropriate variant to use in syntax tests, because even if the most allowed variant is used, the compiler shall still emit errors if it e.g. is used in the wrong places.

r? `@davidtwco` who already [approved ](rust-lang#120832 (review)) this commit in rust-lang#120832.

It would be nice to land the last preparatory commit of that PR before we begin to [rename ](rust-lang#120832 (comment)) things which will of course create a lot of code conflicts.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 11, 2024
…idtwco

unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests

The `sig_dfl` variant of the attribute is the most likely variant to be stabilized first, and thus to become the "most allowed" variant of the attribute. Because of this, it is the most appropriate variant to use in syntax tests, because even if the most allowed variant is used, the compiler shall still emit errors if it e.g. is used in the wrong places.

r? ``@davidtwco`` who already [approved ](rust-lang#120832 (review)) this commit in rust-lang#120832.

It would be nice to land the last preparatory commit of that PR before we begin to [rename ](rust-lang#120832 (comment)) things which will of course create a lot of code conflicts.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024
Rollup merge of rust-lang#122328 - Enselic:sig_dfl-not-inherit, r=davidtwco

unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests

The `sig_dfl` variant of the attribute is the most likely variant to be stabilized first, and thus to become the "most allowed" variant of the attribute. Because of this, it is the most appropriate variant to use in syntax tests, because even if the most allowed variant is used, the compiler shall still emit errors if it e.g. is used in the wrong places.

r? ``@davidtwco`` who already [approved ](rust-lang#120832 (review)) this commit in rust-lang#120832.

It would be nice to land the last preparatory commit of that PR before we begin to [rename ](rust-lang#120832 (comment)) things which will of course create a lot of code conflicts.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 12, 2024
unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests

The `sig_dfl` variant of the attribute is the most likely variant to be stabilized first, and thus to become the "most allowed" variant of the attribute. Because of this, it is the most appropriate variant to use in syntax tests, because even if the most allowed variant is used, the compiler shall still emit errors if it e.g. is used in the wrong places.

r? ``@davidtwco`` who already [approved ](rust-lang/rust#120832 (review)) this commit in rust-lang/rust#120832.

It would be nice to land the last preparatory commit of that PR before we begin to [rename ](rust-lang/rust#120832 (comment)) things which will of course create a lot of code conflicts.
@m-ou-se
Copy link
Member

m-ou-se commented Mar 19, 2024

I'm worried that stabilizing this attribute makes std even more special than it already is. See my comment on the tracking issue: #97889 (comment)

@apiraino apiraino added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2024
@Enselic
Copy link
Member Author

Enselic commented Apr 1, 2024

I will look into ways of removing the sigpipe arg on fn lang_start(). I will also look into renaming the attribute.

This means stabilization of this attribute will not happen near term, so I will close this PR for now. Thanks all for the feedback 👍

@Enselic Enselic closed this Apr 1, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 4, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang#97889
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 4, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang#97889
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 4, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang#97889
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
Rollup merge of rust-lang#124480 - Enselic:on-broken-pipe, r=jieyouxu

Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang#97889
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 4, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang/rust#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang/rust#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang/rust#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang/rust#97889
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request May 18, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang/rust#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang/rust#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang/rust#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang/rust#97889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious "broken pipe" error messages, when used in typical UNIX shell pipelines