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: Go2: add common way to handle operations safely #22796

Closed
hirochachacha opened this issue Nov 18, 2017 · 29 comments
Closed

proposal: Go2: add common way to handle operations safely #22796

hirochachacha opened this issue Nov 18, 2017 · 29 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@hirochachacha
Copy link
Contributor

Currently, map access and type assertions have way to detect unsafe operations before causing runtime panic.
I propose extending the syntax for all operations that may cause runtime panic.

Proposal

  • index access
if x, ok := a[n]; ok {
	...
}

is equivalent to

if 0 <= n && n < len(a) {
	x := a[n]
	...
}
  • slicing / full slicing
if x, ok := a[n:m]; ok {
	...
}

if x, ok := a[n:m:max]; ok {
	...
}

is equivalent to

if 0 <= n && n <= m && m <= len(a) { // use cap(a) for slices
	x := a[n:m]
	...
}

if 0 <= n && n <= m && m <= max && max <= cap(a) {
	x := a[n:m:max]
	...
}
  • pointer access
if foo, ok := p.Foo; ok {
	...
}

is equivalent to

if p != nil {
	foo := p.Foo
	...
}
  • integer division
if z, ok := x/y; ok {
	...
}

is equivalent to

if y != 0 {
	z := x/y
	...
}
  • pointer dereference
if x, ok := *p; ok {
	...
}

is equivalent to

if p != nil {
	x := *p
	...
}
  • dynamic type comparison
if b, ok := iface1 == iface2; ok {
	...
}

no equivalent syntax now.

After defining above semantics, we can define another one.

  • deep checking
if s, ok := out[0].Left.Orig.Sym.Name[:1]; ok && s != "~r" { // ~r%d is the name invented for an unnamed result
	...
}

is equivalent to

// taken from cmd/compile/internal/gc/dcl.go

if len(out) > 0 && out[0].Left != nil && out[0].Left.Orig != nil {
	s := out[0].Left.Orig.Sym
	if s != nil && (s.Name[0] != '~' || s.Name[1] != 'r') { // ~r%d is the name invented for an unnamed result
		...
	}
}

Motivation

I think 'deep checking' example explains the situation.
To write correct code, to review somebody's code,
We have to consider array/slice bounds check, null check, ..., etc.
Those boilerplate are verbose and error prone still today.
The only important question for this kind of problem is 'Is the target attribute accessible or not?'.
The new semantics answer that question clearly without any noise.
We can guarantee that code works as intended without knowing details.

Thank you for reading.

@gopherbot gopherbot added this to the Proposal milestone Nov 18, 2017
@bradfitz bradfitz added v2 A language change or incompatible library change LanguageChange labels Nov 18, 2017
@as
Copy link
Contributor

as commented Nov 18, 2017

If I see a fault that occurs in the deep checking example by what means do i go about determining what generated it short of rewriting the expression in the standard format?

@hirochachacha
Copy link
Contributor Author

@as sorry, I'm not a good reader. What is rewriting the expression in the standard format?
The proposed syntax don't tell where the evaluation failed. It only tell whether the evaluation failed.
The compiler walks in chained expressions left to right, generate some guard per fallible expressions. I guess I'm not answering your question though.

@dmkra
Copy link

dmkra commented Nov 18, 2017

The syntax for allocation primitives new and make can also be extended to detect OOM.

if p, ok := new(int); ok {
}

@adamdecaf
Copy link
Contributor

Shouldn't if 0 <= n && n < len(a) be if 0 >= n && n < len(a) in the first example?

@hirochachacha
Copy link
Contributor Author

@dmkra Go allocates new object as a result of escape analysis.

func foo() *int {
  a := 1
  _ = new(A)
  return &a
}

new(A) doesn't allocate new object, a := 1 does.
Plus, Go's stack is growable. So I think it's more complicated.

@adamdecaf I think 0 >= n is wrong.

@slrz
Copy link

slrz commented Nov 18, 2017

Most of these look like papering over bugs somewhere up the stack, making them quite different from type assertions and map access.

@as
Copy link
Contributor

as commented Nov 18, 2017

@hirochachacha
By standard format I meant the long version of the expression. What I was getting at is that, although this syntax makes it easy to write, it doesn't make it easy to determine where the operation failed:

if x, ok := a[n]; ok {
	...
}

In the expression above, I almost always want to know why it's !ok when there is more than one thing that can go wrong. For maps, it's because the key doesn't map to a value. For type assertions, the answer is also obvious. What about:

if s, ok := out[0].Left.Orig.Sym.Name[:1]; ok && s != "~r" { 
         // ~r%d is the name invented for an unnamed result
	...
}

If it's !ok, how do I find the reason why it's !ok?

@hirochachacha
Copy link
Contributor Author

@slrz maybe.

@as

If it's !ok, how do I find the reason why it's !ok?

You have to split code something like

if o, ok := out[0].Left.Orig; ok {
  if s, ok := o.Sym.Name[:1]; ok && s != "~r" {
    ...
  }
}

The proposed syntax don't tell the location it failed.

@dmkra
Copy link

dmkra commented Nov 19, 2017

@hirochachacha
Extended syntax for new is optional and always allocates on the heap. If a user prefers escape analysis and possible panic then he should use usual syntax.

@as
Copy link
Contributor

as commented Nov 19, 2017

So this means we get a choice of which expression to write. The shorter version removes error context in favor of a shorter expression. I would rather see the landmines this expression hides rather than allowing users the option to hide them.

@hirochachacha
Copy link
Contributor Author

@dmkra I'm not sure the value of OOM detecting, but, yes it could be.

@as I think the best choice depends the problem.

@urandom
Copy link

urandom commented Nov 20, 2017

Perhaps sending to a channel should also have a safe equivalent, in case the channel is closed:

if ok := ch <- 1; !ok {
    ...
}

That would complement the receive case which already has a second component.

@as
Copy link
Contributor

as commented Nov 20, 2017

@urandom

There has been prior discussion on this--specifically that it makes it easier to design a program where the producer/consumer lines are blurred to the point of the program is completely incorrect.

On the other hand, there is one fork maintained by lsub that modifies the language to achieve that behavior. To my knowledge, there has never been a direct discussion of its merits.

@hirochachacha
Copy link
Contributor Author

I admit that this proposal can includes #21985.

specifically that it makes it easier to design a program where the producer/consumer lines are blurred to the point of the program is completely incorrect.

I also admit I don't have counterarguments against the statement. Hmm.

@urandom
Copy link

urandom commented Nov 27, 2017

@as
A lot of previous discussions center around safely closing a closed channel, which would indeed allow people to do bad things such as allow consumers to close a channel. The most recent discussion (#21985) seems to include some bizarre comments that do not seem to be related to the proposal at hand there, which leads me to believe that it was changed after the fact, thus mudding the discussion.

As for blurring the lines between a consumer and producer, I have to admit that I'm not seeing it. How would having a safe send option allow consumers to even closely resemble producers? On the other hand, a typical parallel producer pipeline can be simplified by only having the channel they are sending on, as opposed to having an extra channel that the producers themselves are consumers of.

@as
Copy link
Contributor

as commented Nov 28, 2017

Closing a channel is a signal to the consumer that no more data will be sent through that channel. With that in mind, allowing producers to catch this signal through failed sends to an already closed channel blurs the consumer/producer relationship because that signal is intended to be sent exclusively downstream to the consumers.

@urandom
Copy link

urandom commented Nov 28, 2017

That's currently true, though I'm left to wonder whether this behavior was created out of necessity. Real life doesn't work that way. If you want to ship a package and no one told you the post office is closed today, you don't just throw your package furiously at its closed doors. Yet this is how channels currently work. Not very intuitive for people trying to pick up the language.

@andlabs
Copy link
Contributor

andlabs commented Jan 31, 2018

Actually before Go 1.0 the idiom was

if !closed(c) {
    v = <-c
}

but this was changed to the current comma-ok rule because a goroutine switch between the call to closed and the receive can result in the rug being pulled from under you (TOCTTOU bug).

And I'm not sure about this post office analogy, because the idea of a channel is that you own the post office and can thus decide the hours of operation. How you would model other people trying to give you packages to send to other people I don't know right now.

@nhooyr
Copy link
Contributor

nhooyr commented Feb 3, 2018

What exactly is

dynamic type comparison

Don't we already have that because we can compare interfaces? Why does the result of the comparison need to be a variable and a boolean?

@hirochachacha
Copy link
Contributor Author

@nhooyr interface comparison panic if underlying type is uncomparable.

https://golang.org/ref/spec#Comparison_operators says:

A comparison of two interface values with identical dynamic types causes a run-time panic if values of that type are not comparable. This behavior applies not only to direct interface value comparisons but also when comparing arrays of interface values or structs with interface-valued fields.

For example, https://play.golang.org/p/M3KLDx8OUkE

@nhooyr
Copy link
Contributor

nhooyr commented Feb 3, 2018

Huh, TIL. Thanks!

@hirochachacha
Copy link
Contributor Author

@andlabs IIUC, post office analogy implies actor model or something.
Unlike Erlang, Go doesn't support distributed systems natively.
So I could not find any use-cases yet though.

@as
Copy link
Contributor

as commented Feb 4, 2018

That's currently true, though I'm left to wonder whether this behavior was created out of necessity. Real life doesn't work that way. If you want to ship a package and no one told you the post office is closed today, you don't just throw your package furiously at its closed doors. Yet this is how channels currently work. Not very intuitive for people trying to pick up the language.

I don't think real life works this way either, nobody tells you its closed unless you ask first. If it's open, you give it your mail. A channel is similar to a full-duplex pipe for typed values, and the correct way to orchestrate work through it is similar to a UNIX pipeline.

  1. Check that the post office is open
  2. Prepare packages
  3. Drop off the packages

To drive to the post office first and only find out it's not open is lack of organization and planning. You just wasted all that time preparing the packages and driving down there only to find out it's closed. You should know whether the channel is closed before you send on it.

Lsub go disagrees specifically with the premise (see link provided earlier), and it seems to be a matter of taste as well as simplicity. The disadvantage with lsub go is that it is a system built for experts who understand the tradeoffs a lot better than the average user.

@ianlancetaylor
Copy link
Contributor

For all of the expressions listed, other than interface comparison, the programmer can write an explicit test to avoid the panic. This is not true for the cited examples of map indexes and type assertions.

The explicit tests will produce code that is as good as what the compiler will generate anyhow.

The new forms can not be used as expressions.

It is rare for a slice expression to use an index that might be invalid. Generally you already know that the index is valid anyhow. In the cases where you don't know, the index check condition is often combined with other conditions anyhow, which can not be done using this new form. For instance, for i < len(a) && i < len(b) { ... }.

Declined.

@urandom
Copy link

urandom commented Mar 28, 2018

@ianlancetaylor
How would you write a test for safely sending data through a channel that is not prone to race conditions?

@as
Copy link
Contributor

as commented Mar 28, 2018

Guard the channel with another--an inverted semaphore representing the capability to cleanup resources one time works well and replaces sync.Once.

scram := make(chan bool, 1)
out := make(chan work, 1024)
scram <-true   // happens exactly once

func run(){
   for{
      select{
      case first := <-scram :
         if first{
               close(scram)
               close(out)
               // cleanup, drain, etc
         }
        return
     default:
          out <- item{}
  }
}

@ianlancetaylor
Copy link
Contributor

I'm probably missing something but it seems to me that you can just use a select statement.

    sent := false
    select {
    case c <- v:
        sent = true
    default:
    }

If you want to ask whether it is possible to send a value without actually sending the value, that is inherently racy no matter how it is implemented.

@urandom
Copy link

urandom commented Mar 28, 2018

@ianlancetaylor
The idea is to know whether the message was sent without getting a panic if it was not. The runtime already has this information, since it panics, but the user has to do quite a bit of work in order to avoid the panic.

@ianlancetaylor
Copy link
Contributor

Oh, sorry, you meant sending on a closed channel. That is typically a logic error. Clearly there are already ways to do this where it is required, such as a mutex. I don't think it's necessary to modify the language to make it easier to handle.

@golang golang locked and limited conversation to collaborators Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests