fix: Hub.PopScope never empties the scope stack (#300)

Equivalent change in JS:
https://togithub.com/getsentry/sentry-javascript/pull/2955

hub.go

@@ -92,30 +92,21 @@ func (hub *Hub) LastEventID() EventID {
 	return hub.lastEventID
 }
 
+// stackTop returns the top layer of the hub stack. Valid hubs always have at
+// least one layer, therefore stackTop always return a non-nil pointer.
 func (hub *Hub) stackTop() *layer {
 	hub.mu.RLock()
 	defer hub.mu.RUnlock()
 
 	stack := hub.stack
-	if stack == nil {
-		return nil
-	}
-
 	stackLen := len(*stack)
-	if stackLen == 0 {
-		return nil
-	}
 	top := (*stack)[stackLen-1]
-
 	return top
 }
 
 // Clone returns a copy of the current Hub with top-most scope and client copied over.
 func (hub *Hub) Clone() *Hub {
 	top := hub.stackTop()
-	if top == nil {
-		return nil
-	}
 	scope := top.scope
 	if scope != nil {
 		scope = scope.Clone()
@@ -126,32 +117,21 @@ func (hub *Hub) Clone() *Hub {
 // Scope returns top-level Scope of the current Hub or nil if no Scope is bound.
 func (hub *Hub) Scope() *Scope {
 	top := hub.stackTop()
-	if top == nil {
-		return nil
-	}
 	return top.scope
 }
 
 // Client returns top-level Client of the current Hub or nil if no Client is bound.
 func (hub *Hub) Client() *Client {
 	top := hub.stackTop()
-	if top == nil {
-		return nil
-	}
 	return top.Client()
 }
 
 // PushScope pushes a new scope for the current Hub and reuses previously bound Client.
 func (hub *Hub) PushScope() *Scope {
 	top := hub.stackTop()
 
-	var client *Client
-	if top != nil {
-		client = top.Client()
-	}
-
 	var scope *Scope
-	if top != nil && top.scope != nil {
+	if top.scope != nil {
 		scope = top.scope.Clone()
 	} else {
 		scope = NewScope()
@@ -161,31 +141,37 @@ func (hub *Hub) PushScope() *Scope {
 	defer hub.mu.Unlock()
 
 	*hub.stack = append(*hub.stack, &layer{
-		client: client,
+		client: top.Client(),
 		scope:  scope,
 	})
 
 	return scope
 }
 
-// PopScope pops the most recent scope for the current Hub.
+// PopScope drops the most recent scope.
+//
+// Calls to PopScope must be coordinated with PushScope. For most cases, using
+// WithScope should be more convenient.
+//
+// Calls to PopScope that do not match previous calls to PushScope are silently
+// ignored.
 func (hub *Hub) PopScope() {
 	hub.mu.Lock()
 	defer hub.mu.Unlock()
 
 	stack := *hub.stack
 	stackLen := len(stack)
-	if stackLen > 0 {
+	if stackLen > 1 {
+		// Never pop the last item off the stack, the stack should always have
+		// at least one item.
 		*hub.stack = stack[0 : stackLen-1]
 	}
 }
 
 // BindClient binds a new Client for the current Hub.
 func (hub *Hub) BindClient(client *Client) {
 	top := hub.stackTop()
-	if top != nil {
-		top.SetClient(client)
-	}
+	top.SetClient(client)
 }
 
 // WithScope runs f in an isolated temporary scope.

hub_test.go

@@ -83,13 +83,11 @@ func TestPopScopeRemovesLayerFromTheStack(t *testing.T) {
 	assertEqual(t, len(*hub.stack), 2)
 }
 
-func TestPopScopeCannotRemoveFromEmptyStack(t *testing.T) {
+func TestPopScopeCannotLeaveStackEmpty(t *testing.T) {
 	hub, _, _ := setupHubTest()
 	assertEqual(t, len(*hub.stack), 1)
 	hub.PopScope()
-	assertEqual(t, len(*hub.stack), 0)
-	hub.PopScope()
-	assertEqual(t, len(*hub.stack), 0)
+	assertEqual(t, len(*hub.stack), 1)
 }
 
 func TestBindClient(t *testing.T) {
@@ -192,27 +190,6 @@ func TestLastEventIDUpdatesAfterCaptures(t *testing.T) {
 	assertEqual(t, *eventID, hub.LastEventID())
 }
 
-func TestLayerAccessingEmptyStack(t *testing.T) {
-	hub := &Hub{}
-	if hub.stackTop() != nil {
-		t.Error("expected nil to be returned")
-	}
-}
-
-func TestLayerAccessingScopeReturnsNilIfStackIsEmpty(t *testing.T) {
-	hub := &Hub{}
-	if hub.Scope() != nil {
-		t.Error("expected nil to be returned")
-	}
-}
-
-func TestLayerAccessingClientReturnsNilIfStackIsEmpty(t *testing.T) {
-	hub := &Hub{}
-	if hub.Client() != nil {
-		t.Error("expected nil to be returned")
-	}
-}
-
 func TestAddBreadcrumbRespectMaxBreadcrumbsOption(t *testing.T) {
 	hub, client, scope := setupHubTest()
 	client.options.MaxBreadcrumbs = 2