Clang wrongly allows overriding method to vary its return type

[pkasting]

The following code generates no warnings in Clang despite the override's return type being incorrect:

struct S {
  virtual const void* f() = 0;
  virtual ~S() = default;  // Silence -Wnon-virtual-dtor
};

struct T : S {
  void* f() override;  // Should warn or error
};

Other compilers:
gcc: Warns
MSVC: Errors
Live example: https://godbolt.org/z/TjoxEzd7M

Larger example demonstrating dispatch in the presence of overloads: https://godbolt.org/z/orcfbrnGz

[pkasting]

Note that if T's override is changed from void* to int*, Clang refuses to compile: error: return type of virtual function 'f' is not covariant with the return type of the function it overrides ('int *' is not derived from 'const void *')

[shafik]

I believe we are suppose to diagnose this here:

llvm-project/clang/lib/Sema/SemaDeclCXX.cpp

Lines 18328 to 18337 in 4aba20f

	// The qualifiers of the return types must be the same.
	if (NewTy.getLocalCVRQualifiers() != OldTy.getLocalCVRQualifiers()) {
	Diag(New->getLocation(),
	diag::err_covariant_return_type_different_qualifications)
	<< New->getDeclName() << NewTy << OldTy
	<< New->getReturnTypeSourceRange();
	Diag(Old->getLocation(), diag::note_overridden_virtual_function)
	<< Old->getReturnTypeSourceRange();
	return true;
	}

I have not dug further in yet to see why.

[zmodem]

@ilya-biryukov maybe someone from your team could take a look?

[ilya-biryukov]

@zmodem thanks for pointing me to it, seems like a good fit for the new people who are ramping up, I'll see if we can find someone.

[ilya-biryukov]

The relevant rule in the standard is here: https://eel.is/c++draft/class.virtual#8

The return type of an overriding function shall be either identical to the return type of the overridden function or covariant with the classes of the functions. If a function D​::​f overrides a function B​::​f, the return types of the functions are covariant if they satisfy the following criteria:
(8.1)

• both are pointers to classes, both are lvalue references to classes, or both are rvalue references to classes96
• the class in the return type of B​::​f is the same class as the class in the return type of D​::​f, or is an unambiguous and accessible direct or indirect base class of the class in the return type of D​::​f
• both pointers or references have the same cv-qualification and the class type in the return type of D​::​f has the same cv-qualification as or less cv-qualification than the class type in the return type of B​::​f.

I think the crux of the issue is that Clang implements the last bullet point, treating the less qualified types as covariant, but does not check the type being pointed to is void, but not a class type (first bullet point).
This seems like a fairly small thing as this shouldn't actually result in any runtime failures, even if Clang rejects some standard-conforming code. But it should also be easy to fix.

@shafik the code in question seems to compare the qualifiers of the top-level types, but to implement the rule above we should compare qualifiers of the types under the pointer, hence the behavior we're seeing.

// NewTy is 'void*'
// OldTy is 'const void*'
// This line is true because top-level qualifiers for both of the types are empty
// (there aren't any qualifiers on top of the pointer, i.e. it's not `void*const`)
NewTy.getLocalCVRQualifiers() != OldTy.getLocalCVRQualifiers())

(I believe the top-level qualifiers should also be the same, someone should dig deeper)

[ilya-biryukov]

I'm booking this for @bricknerb, who isn't part of the LLVM project yet, but will be looking at that issue.