internal/impl: ensure proto.HasExtension does not allocate

Extensions are unmarshaled lazily if protolegacy is true. The current
implementation of proto.HasExtension forces this unmarshaling to happen.
Change that.

Lazy message extensions are unmarshaled on first access, see
(*ExtensionField).Value. This leads to an (expensive) unmarshal
operation even if the user only wanted to know whether the extension is
present.

Granted, in most cases a HasExtension returning true will be followed by
a GetExtension. Due to memoization (see (*ExtensionField).lazyInit), the
cost will just shift from HasExtension to GetExtension. But, this CL
allows building cheaper functionality that only needs to know about
extension existence.

Why can this validation be removed?

 - All tests pass.
 - This check was added in CL 229558. The author (Joe Tsai) noted:

> Technically this shouldn't be needed, but I couldn't adequately reason
> whether a nil message value would ever be set through non-reflection
> means.

Like the author, I believe it's not needed:

 - `proto.SetExtension` does not allow setting invalid messages (see
   proto/extension.go).
 - Likewise, (*extensionMap).Set panics when attempting to set an
   invalid value.
 - Unmarshaling does not produce submessages for which `IsValid` is
   false.

The added test fails without the fix:

    $ go test -tags=protolegacy -test.v -test.run=TestHasExtensionNoAlloc proto/extension_test.go
    === RUN   TestHasExtensionNoAlloc
    === RUN   TestHasExtensionNoAlloc/Nil
    === RUN   TestHasExtensionNoAlloc/Eager
    === RUN   TestHasExtensionNoAlloc/Lazy
        extension_test.go:156: proto.HasExtension should not allocate, but allocated 3.00B per run
    --- FAIL: TestHasExtensionNoAlloc (0.00s)
        --- PASS: TestHasExtensionNoAlloc/Nil (0.00s)
        --- PASS: TestHasExtensionNoAlloc/Eager (0.00s)
        --- FAIL: TestHasExtensionNoAlloc/Lazy (0.00s)
    FAIL
    FAIL    command-line-arguments  0.018s

The tests are disabled in race mode because the race instrumentation for
closures et al. always allocates. The protolegacy tests were previously
only run in race mode. I added a non-race variant in
integration_test.go.

Change-Id: Idbc67c1cf0aea8833a2735ca7bfc8d2466ceaf44
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575035
Reviewed-by: Nicolas Hillegeer <aktau@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Auto-Submit: Nicolas Hillegeer <aktau@google.com>

integration_test.go

@@ -145,7 +145,8 @@ func TestIntegration(t *testing.T) {
 		runGo("PureGo", command{}, "go", "test", "-race", "-tags", "purego", "./...")
 		runGo("Reflect", command{}, "go", "test", "-race", "-tags", "protoreflect", "./...")
 		if goVersion == golangLatest {
-			runGo("ProtoLegacy", command{}, "go", "test", "-race", "-tags", "protolegacy", "./...")
+			runGo("ProtoLegacyRace", command{}, "go", "test", "-race", "-tags", "protolegacy", "./...")
+			runGo("ProtoLegacy", command{}, "go", "test", "-tags", "protolegacy", "./...")
 			runGo("ProtocGenGo", command{Dir: "cmd/protoc-gen-go/testdata"}, "go", "test")
 			runGo("Conformance", command{Dir: "internal/conformance"}, "go", "test", "-execute")
 

internal/impl/message_reflect.go

@@ -261,8 +261,6 @@ func (m *extensionMap) Has(xt protoreflect.ExtensionType) (ok bool) {
 		return x.Value().List().Len() > 0
 	case xd.IsMap():
 		return x.Value().Map().Len() > 0
-	case xd.Message() != nil:
-		return x.Value().Message().IsValid()
 	}
 	return true
 }

internal/test/race/race_no.go

@@ -0,0 +1,10 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build !race
+
+package race
+
+// Enabled is true if the race detector is enabled.
+const Enabled = false

internal/test/race/race_yes.go

@@ -0,0 +1,12 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build race
+
+// Package race exposes Enabled, a const indicating whether the test is running
+// under the race detector.
+package race
+
+// Enabled is true if the race detector is enabled.
+const Enabled = true

proto/extension_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 
+	"google.golang.org/protobuf/internal/test/race"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/runtime/protoimpl"
@@ -100,6 +101,64 @@ func TestExtensionFuncs(t *testing.T) {
 	}
 }
 
+func TestHasExtensionNoAlloc(t *testing.T) {
+	// If extensions are lazy, they are unmarshaled on first use. Verify that
+	// HasExtension does not do this by testing that it does not allocation. This
+	// test always passes if extension are eager (the default if protolegacy =
+	// false).
+	if race.Enabled {
+		t.Skip("HasExtension always allocates in -race mode")
+	}
+	// Create a message with a message extension. Doing it this way produces a
+	// non-lazy (eager) variant. Then do a marshal/unmarshal roundtrip to produce
+	// a lazy version (if protolegacy = true).
+	want := int32(42)
+	mEager := &testpb.TestAllExtensions{}
+	proto.SetExtension(mEager, testpb.E_OptionalNestedMessage, &testpb.TestAllExtensions_NestedMessage{
+		A:           proto.Int32(want),
+		Corecursive: &testpb.TestAllExtensions{},
+	})
+
+	b, err := proto.Marshal(mEager)
+	if err != nil {
+		t.Fatal(err)
+	}
+	mLazy := &testpb.TestAllExtensions{}
+	if err := proto.Unmarshal(b, mLazy); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, tc := range []struct {
+		name string
+		m    proto.Message
+	}{
+		{name: "Nil", m: nil},
+		{name: "Eager", m: mEager},
+		{name: "Lazy", m: mLazy},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			// Testing for allocations can be done with `testing.AllocsPerRun`, but it
+			// has some snags that complicate its use for us:
+			//  - It performs a warmup invocation before starting the measurement. We
+			//    want to skip this because lazy initialization only happens once.
+			//  - Despite returning a float64, the returned value is an integer, so <1
+			//    allocations per operation are returned as 0. Therefore, pass runs =
+			//    1.
+			warmup := true
+			avg := testing.AllocsPerRun(1, func() {
+				if warmup {
+					warmup = false
+					return
+				}
+				proto.HasExtension(tc.m, testpb.E_OptionalNestedMessage)
+			})
+			if avg != 0 {
+				t.Errorf("proto.HasExtension should not allocate, but allocated %.2fx per run", avg)
+			}
+		})
+	}
+}
+
 func TestIsValid(t *testing.T) {
 	tests := []struct {
 		xt   protoreflect.ExtensionType