This is the feedback thread for the Final Review of DIP 1034, "Add a Bottom Type (reboot)".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules outlined in the Reviewer Guidelines (and listed at the bottom of this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback to a DIP review. Please read it before posting here. If you would like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/heylgwkzcpfqmqytiezq@forum.dlang.org

==================================

You can find DIP 1034 here:

https://github.com/dlang/DIPs/blob/b5bf9c4bed7afdae3bea1485093dbf2431bf72c7/DIPs/DIP1034.md

The review period will end at 11:59 PM ET on October 6, or when I make a post declaring it complete. Feedback posted to this thread after that point may be ignored.

At the end of the Final Review, the DIP will be forwarded to the language maintainers for Formal Assessment.

==================================
Posts in this thread that do not adhere to the following rules will be deleted at the DIP author's discretion:

* All posts must be a direct reply to the DIP manager's initial post, with only two exceptions:

    - Any commenter may reply to their own posts to retract feedback contained in the original post

    - The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case)

* Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.

* Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review (not Final Review), but must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in the reviewer guidelines will receive much gratitude). Information which irrelevant to the DIP or is not provided in service of clarifying the feedback is unwelcome.

On Tuesday, 22 September 2020 at 12:17:09 UTC, Mike Parker wrote:

> https://github.com/dlang/DIPs/blob/b5bf9c4bed7afdae3bea1485093dbf2431bf72c7/DIPs/DIP1034.md

****PLEASE NOTE****
I apologize. The above is the wrong link. The correct link is here:

https://github.com/dlang/DIPs/blob/d492715d3c7ba2cee898930d261ab113c0a66ef9/DIPs/DIP1034.md

On Tuesday, 22 September 2020 at 12:25:17 UTC, Mike Parker wrote:
> On Tuesday, 22 September 2020 at 12:17:09 UTC, Mike Parker wrote:
>
>> https://github.com/dlang/DIPs/blob/b5bf9c4bed7afdae3bea1485093dbf2431bf72c7/DIPs/DIP1034.md
>
> ****PLEASE NOTE****
> I apologize. The above is the wrong link. The correct link is here:
>
> https://github.com/dlang/DIPs/blob/d492715d3c7ba2cee898930d261ab113c0a66ef9/DIPs/DIP1034.md

The formatting of the unordered list under "Community Review Round 1" is broken.

On Tuesday, 22 September 2020 at 12:40:54 UTC, Paul Backus wrote:
https://github.com/dlang/DIPs/blob/d492715d3c7ba2cee898930d261ab113c0a66ef9/DIPs/DIP1034.md
>
> The formatting of the unordered list under "Community Review Round 1" is broken.

Thanks. I'll fix that at the end of the review period.

Well written and very compelling!

I have a couple feedback points:

1. In "The cast operator":

```
int x;
int bar();
auto foo() {
    cast(noreturn) (x = bar()); // same as {x = bar(); assert(0);}
}
```

This is odd, and I'm not understanding the reason for the cast if you aren't assigning.

Also, earlier you say that you can declare a noreturn variable, but as long as you don't use it, you don't generate the assert 0. Is this example a contradiction of that?

2. Inside Description, change (3) "Implicit conversions from `noreturn` to any other type are allowed", I am having a difficult time with rules 6 and 7:

```
/* 6 */ !(is(T == function)) || is(noreturn* : T)
/* 7 */ !(is(T == delegate)) || is(noreturn* : T)
```

What are these trying to say? I think these rules need some clarification or rewriting

-Steve

On Tuesday, 22 September 2020 at 14:16:19 UTC, Steven Schveighoffer wrote:
> ```
> int x;
> int bar();
> auto foo() {
>     cast(noreturn) (x = bar()); // same as {x = bar(); assert(0);}
> }
> ```
>
> This is odd, and I'm not understanding the reason for the cast if you aren't assigning.

This code has no reason, it's just an example of the rewrite that happens with `cast(noreturn)`. It could actually be useful in generic code, or when you are using a function that is noreturn but does not have the correct return type.
```
// someone else's code
void panic(string msg);

// your code:
auto f = () => cast(noreturn) panic("error");
```

> Also, earlier you say that you can declare a noreturn variable, but as long as you don't use it, you don't generate the assert 0. Is this example a contradiction of that?

I don't think so, the example does not declare a local variable. I could rewrite it to this:

```
import std;
int x;
int bar();
auto foo() {
    noreturn y; // no assert here yet
    writeln("this will get printed");
    y = cast(noreturn) (x = bar());
    // here it results in assert(0)
}
```

The reason for this is argued by Johannes Loher here:
https://forum.dlang.org/post/r8v8tt$14fk$1@digitalmars.com

> ```
> /* 6 */ !(is(T == function)) || is(noreturn* : T)
> /* 7 */ !(is(T == delegate)) || is(noreturn* : T)
> ```
>
> What are these trying to say? I think these rules need some clarification or rewriting

In formal logic, "(not p) or q" is equivalent to "if p then q", so it means "if T is a function/delegate, noreturn* implicitly converts to it". The DIP follows the rules up with a short explanation:

> 5, 6 and 7 mean that null (which is of type noreturn*) may still be assigned to a function pointer or array.

The point is that these things should still be allowed when `typeof(null)` becomes equivalent to `noreturn*`:
```
int[] a = null;
void function() f = null;
void delegate() d = null;
Object o = null;
```
(I just realized I have no rule for `T == class`. I sometimes forget classes exist in D :p)

I'll try to make it clearer.

On Tuesday, 22 September 2020 at 12:17:09 UTC, Mike Parker wrote:
> This is the feedback thread for the Final Review of DIP 1034, "Add a Bottom Type (reboot)".

From the DIP:

>  * A union has the size of the largest member. Adding a noreturn field to a union never increases the size, so its size is 0.

Currently, all structs and unions must have .init values that are computable at compile time, which means that adding a noreturn field would have to be a compile-time error, since it makes the .init value impossible to compute. You should probably specify in the section about `noreturn.init` than any aggregate with a noreturn field also has no .init value.

On Tuesday, 22 September 2020 at 17:24:02 UTC, Paul Backus wrote:
> Currently, all structs and unions must have .init values that are computable at compile time, which means that adding a noreturn field would have to be a compile-time error, since it makes the .init value impossible to compute.

You can still compute an init value, it'd be the same bytes as when the noreturn fields weren't there. I don't think foo0 and foo1 should act differently here:

```
struct S {
    int x;
    noreturn y;
    double z;
}

void foo0() {
    S s;
}

void foo1() {
    int x;
    noreturn y;
    double z;
}
```