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

Specify error for a function returning Never which may return #913

Closed
wants to merge 2 commits into from

Conversation

eernstg
Copy link
Member

@eernstg eernstg commented Apr 3, 2020

[Edit, eernstg: I'm closing this PR because one change is already subsumed by existing rules in the language specification, and I'd prefer to avoid the other change (and allow certain programs), and this PR only contained those two changes.]

It seems reasonable to flag a function that may return (like return; or return d; where d has type dynamic) if it has return type Never. I put this error into and just after the error that catches the situation where these functions (along with other functions whose return type is potentially non-nullable) may complete normally.

Cf. #914 as well.

leafpetersen
leafpetersen previously approved these changes Apr 4, 2020
@leafpetersen leafpetersen self-requested a review April 4, 2020 00:51
@leafpetersen leafpetersen dismissed their stale review April 4, 2020 00:52

On further thought, I'm not sure that I agree with these changes.

return without an object.

It is an error if the body of a method, function, getter, or function expression
with return type `Never` may return with an object.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is necessary, nor desirable.

Never bar() => throw "Unreachable";
Never foo() {return bar();}

is a good program, and should be supported, I think.

dynamic bar() => throw "Unreachable";
Never foo() {return bar();}

is a bad program, but the reason has nothing to do with Never. There's just an implicit downcast there from dyanamic to Never.

I think this program:

Never foo() {return;}

does need some further specification.

Copy link
Member Author

@eernstg eernstg Apr 6, 2020

Choose a reason for hiding this comment

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

Never bar() => throw "Unreachable";
Never foo() { return bar(); }

This can be defended because it should always be OK to return something of type T when the return type is T. It does have some of the same weirdness as void f() { return print("Hello!"); }, in that it is misleading to read return .. and think "a value is returned here". But we allow that exactly because it returns void to void, so we should probably accept a similar weirdness with Never as well. So I agree that this is a good program, albeit somewhat weird.

dynamic bar() => throw "Unreachable";
Never foo() { return bar(); }

However, I'm not convinced that this is a bad program. We just decided that it will be allowed with null-safety enabled to implicitly cast from dynamic to any type. So why should a cast from dynamic to Never be an exception?

I argue in #914 that we would then trust the expression of static type dynamic to loop forever, or to throw. But this is no more risky than trusting an expression d of static type dynamic to evaluate to an object whose interface has any given member, say, with d.fooBar(42), or trusting it to evaluate to an object of any particular type, as in int i = d;.

So I actually think the consistent approach would be to allow an expression of static type dynamic to be assigned to a variable/parameter of type Never.

In any case, if that expression of static type dynamic does not live up to the expectation (and it does evaluate to an object), then there will be a dynamic check, and we will get an exception at run time.

Never foo() { return; }

As I argued above (at line 354), this is already covered by the rules in \section{Return} of the language specification. So I actually don't think we need more, to make that an error.

But this would mean that I'll just revert this change as well, which means that the whole PR goes away, but then we conclude that line 7 here is not an error (that's Never test3() => getNull();).

Lines 5 and 6 can be non-errors for a similar reason, based on the discussion about return rules in #914.

WDYT?

@eernstg
Copy link
Member Author

eernstg commented Apr 6, 2020

Closing this PR because it is empty. The discussion (especially here) gives further reasons.

@eernstg eernstg closed this Apr 6, 2020
@eernstg eernstg deleted the add_error_Never_return_apr20 branch April 7, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants