go/types2, types2: version check (Checker.allowVersion) ignores module version if position information is missing

[griesemer]

The code in Checker.allowVersion:

	// If no explicit file version is specified,
	// fileVersion corresponds to the module version.
	var fileVersion goVersion
	if pos := at.Pos(); pos.IsKnown() {
		// We need version.Lang below because file versions
		// can be (unaltered) Config.GoVersion strings that
		// may contain dot-release information.
		fileVersion = asGoVersion(check.versions[base(pos)])
	}
	return !fileVersion.isValid() || fileVersion.cmp(v) >= 0

returns true if pos is not known: fileVersions won't be set and thus is not valid, resulting in a true return value.

This leads to correct behavior if a position is missing because it belongs to an imported object/type: in those cases we don't need to do a version check (== any version is allowed). But it may hide problems due to incorrectly missing position information.

Instead, the code should resort to the module version if no position is provided, and code that is version checking should not do the version checks if the checks are not needed in the first place (for imported types/objects).

This code exists also in 1.22 and it hides issue #66064 (in 1.22) because the missing position information effectively leads to an absent version check (which is what we want).

A more robust approach is to explicitly disable the version check if none is needed, and assert that in all other cases we have a valid position.

cc: @findleyr for visibilty

[griesemer]

This was addressed by a) explicit checks to calls to allow/verifyVersion where we know that position information may be missing because of import, b) adjustments to the code of allowVersion, and c) still allowing for unknown positions because parts of the AST may be generated/constructed outside a parser.

[adonovan]

This was addressed by a) explicit checks to calls to allow/verifyVersion where we know that position information may be missing because of import, b) adjustments to the code of allowVersion, and c) still allowing for unknown positions because parts of the AST may be generated/constructed outside a parser.

I don't think (c) is correct. Consider an ast.File stitched together from two parts (whether synthesized from scratch or spliced from two calls to the parser), the first part specifying a FileVersion and the second containing syntax that requires a version check. Depending on the pos values, the type checker would fail to associate the two and would use the wrong semantics. I think file version information needs to be propagated top down; see #69477.

[griesemer]

@adonovan ACK. It's unfortunate that we need to rely on position information in the first place. Maybe we need a position-independent mechanism to identify which file a node belongs to (I suppose one could search for the node in the AST for each file, but that seems pretty expensive).

[adonovan]

@adonovan ACK. It's unfortunate that we need to rely on position information in the first place. Maybe we need a position-independent mechanism to identify which file a node belongs to (I suppose one could search for the node in the AST for each file, but that seems pretty expensive).

I don't think it's technically hard to implement the fix. In essence, we remember the Checker's current file version on the downwards traversal, so that it becomes part of the context saved in each "later" closure. Each later closure (or the later mechanism itself) would re-set the checker's file version before executing the logic.