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

proposal: errors: add String #39493

Closed
carnott-snap opened this issue Jun 10, 2020 · 11 comments
Closed

proposal: errors: add String #39493

carnott-snap opened this issue Jun 10, 2020 · 11 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jun 10, 2020

Proposal

Many times I find my self reimplementing this type to provide constant errors:

package errors

type String string

func (s String) Error() string { return string(s) }

I think this would a benefit to add to the standard library to allow for constant errors:

const ErrSentinal errors.String = "an error occurred"

Costs

This does have the possibility of confusing users, as it overlaps with the use case for errors.errorString/Errors.New:

func string() error {
        return errors.String("failed to foo")
}

func new() error {
        return errors.New("failed to foo")
}

Extensions

To simplify errors, we could also remove errors.errorString in favour of errors.String, within errors.New:

package errors

func New(text string) error { return String(text) }

I am not sold on the benefit of this change, other than maybe moving symbols to the data block? Plus, this seems like a big move and removes the possibility extending errors.errorString allowing it to store mutable state.

Alternatives

As an alternative, this symbol could be named errors.Const to signal the intended usage, as opposed to its implementation as a string:

const ErrSentinel errors.Const = "something went wrong"

To resolve confusion about errors.errorString, while also breaking the Go1 compatibility guarantee, this completely clears up the matter and maintains call site compatibility:

package errors

type New string

func (n New) Error() string { return string(s) }
func fail() error {
        return errors.New("failed to foo")
}

P.S. Credit to Dave Cheney for the inspiration.
P.P.S I have published this as part of my expanding errors wrapper package since this does not involve any language changes.

@gopherbot gopherbot added this to the Proposal milestone Jun 10, 2020
@gopherbot gopherbot added the Proposal label Jun 10, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 10, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

This is the semantics that errors.New used to have, before Go 1, and we explicitly rejected that implementation, to avoid errors defined in two different packages with the same text accidentally being equal. I don't think we want to put that bug-prone behavior back under a different name, nor under the same name.

You've given no reason why it would be beneficial for errors to be constants. Certainly comparing them would be slower (string compare instead of pointer compare).

I don't remember whether we've optimized errors.New in the compiler so that a global error initialized using errors.New doesn't cause any init time work. We should do that if we haven't already. But doesn't require any visible semantic changes or new API.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 10, 2020

This is the semantics that errors.New used to have before Go 1.

Interesting, I was not aware of the history, thanks for the context.

You've given no reason why it would be beneficial for errors to be constants. Certainly comparing them would be slower.

Apologies for not making this more clear, my main motivation is to have constant (read: immutable) error sentinels. For me, this outweighs minor perf costs.

You can argue that we are all consenting adults, but then we would not need private symbols or that monkey patching functions is good, as is done in Python. I would also be happy with const var or similar immutability alternatives.

I don't remember whether we've optimized errors.New in the compiler. We should do that if we haven't already.

Assuming this has not already been done, is it worth spinning off a new issue to track?

@mvdan
Copy link
Member

@mvdan mvdan commented Jun 10, 2020

I agree with the sentiment. Being able to break the world by doing io.EOF = nil is surprising. One should always vet direct and indirect module dependencies, but global errors still feel like a way to break the world that feels unnecessary.

Having said that, if you're interested in read-only variables, you might be interested in previous proposals like #22876 or #21130. If Go expands its support for constants or read-only declarations, it's probably best to not limit our scope to just errors.

@golang golang deleted a comment Jun 10, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

If the problem is unwanted assigns to global variables, let's work on that instead of focusing on the specific case of errors. For errors, especially given the history, I don't think it makes sense to introduce a second error constructor. Having one is at least 10X better than having two, even if a second might provide marginal benefit.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 10, 2020

I guess I thought all of those discussions petered out or were shutdown. If there is active interest and movement on that front, I am game. This was something I saw as feasible today, without sweeping language changes, but I agree there are some caveats.

That being said, I still think there is some benefit to this symbol and would be interested to see the community response, so I will refrain from withdrawing the proposal for now.

@dpinela
Copy link
Contributor

@dpinela dpinela commented Jun 10, 2020

@rsc

I don't remember whether we've optimized errors.New in the compiler so that a global error initialized using errors.New doesn't cause any init time work. We should do that if we haven't already. But doesn't require any visible semantic changes or new API.

It seems we're partway there; see #30820.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 17, 2020

Retracted by @carnott-snap above.

@rsc rsc closed this Jun 17, 2020
@rsc rsc moved this from Active to Declined in Proposals Jun 17, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 17, 2020

I will refrain from withdrawing the proposal for now.

@rsc: I explicitly did not retract my proposal and would like to leave it open for further community discussion.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 18, 2020

Sorry for the confusion. Reopening. But this will just become a likely decline at the next meeting.

@ianlancetaylor ianlancetaylor moved this from Declined to Active in Proposals Jun 18, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

Sorry about claiming it was retracted - misread the ("refrain from withdrawing") comment above.

Now, based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Jun 24, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Jul 8, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

No change in consensus, so declined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.