#1040 Add a warning about implicit conversion of null pointer constants to pointer-to-non-_Optional

[Christopher Bazley]

Annex B of the draft _Optional technical specification is non-normative but it gives examples of additional diagnostic messages that implementations can usefully produce:

• implicit conversion of a null pointer constant to a pointer type whose referenced type is not optional-qualified
• pointer whose referenced type is not optional-qualified is subject to default initialization.
• implicit conversion of a pointer to any element of the array produced from a sequence of string literals to a pointer type whose referenced type is not const-qualified.

The examples can serve as test cases, e.g.

char *npc = nullptr; // valid but unsafe
char *lpc = "hello"; // valid but unsafe
_Optional char *npoc = nullptr; // valid and safe
const char *lpcc = "hello"; // valid and safe

SDCC 4.6.0 produces a warning about the second initialisation but not the first:

<source>:2: warning 360: string literal assigned to pointer to non-const

See https://godbolt.org/z/j5P3dEK13

The usefulness of the _Optional qualifier would be considerably enhanced if both initialisations produced a warning. At first, it is expected that the warning about the second assignment would need to be explicitly enabled per-translation unit, e.g. by a new command line option such as -Wnull-to-nonoptional.

This would be much safer than redefining the NULL macro as (_Optional void *)0 because it does not rely on the spelling of the null pointer constant or an appropriate redefinition having been included.

Pedantically, 0 cast to pointer-to-qualified-type is not a valid type of expression to assign to a function pointer; however, SDCC already allows that:

int (*foo)(int *) = (const void *)0; // no SDCC warning

https://godbolt.org/z/47TrsPv9h

Specifically, this is because:

A null pointer constant is an expression that, after generic replacement and removal of all surrounding balanced parentheses, is an integer constant expression with the value 0 or such an expression cast to type void *, or the predefined constant nullptr.

(6.3.3.3p3 in the C2Y working draft: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3886.pdf)

[Christopher Bazley]

@spth I know you were in favour of warning about implicit conversions of string literals to non-const. Is that why there is (apparently) no way of enabling a disabled warning? What are your feelings about the proposed _Optional warning? Might it be acceptable to enable that by default too? Otherwise a new configuration mechanism would be needed to enable it via a new command line option or #pragma.

[Christopher Bazley]

The attached patch developed with Codex does exactly what I'd expect:

~/sdcc-code/sdcc $ bin/sdcc -c --enable-warning 244 --disable-warning 244  test.c
~/sdcc-code/sdcc $ bin/sdcc -c --enable-warning 244  test.c
test.c:3: warning 244: pointer types incompatible 
from type 'char generic* fixed'
  to type 'int generic* fixed'
~/sdcc-code/sdcc $ bin/sdcc -c --disable-warning 244  test.c
~/sdcc-code/sdcc $ bin/sdcc -c test.c
test.c:3: warning 244: pointer types incompatible 
from type 'char generic* fixed'
  to type 'int generic* fixed'
~/sdcc-code/sdcc $ bin/sdcc -c --disable-warning 244 --enable-warning 244  test.c
test.c:3: warning 244: pointer types incompatible 
from type 'char generic* fixed'
  to type 'int generic* fixed'
~/sdcc-code/sdcc $ 

It also adds support/valdiag/tests/enable-warning.c to verify that #pragma enable_warning behaves as expected and to verify its interactions with #pragma disable_warning.

All regression tests passed on z80 and all valdiag tests (including the new ones) passed on a variety of targets. @spth, judging by your comment on the mailing list, I think you were in favour of this, so would you mind reviewing the patch when you get time? Thanks.

[Philipp Klaus Krause]

I didn't test it, but the patch looks fine to me, except for the comment in the test: there, I'd recommend to just state that it tests enabling and disabling of warnings, and not mention _Optional at all; having the test not depend on _Optional is good, but stating that explicitly increases mental load of readers for those who don't know about _Optional yet.

[Christopher Bazley]

Thanks for the review, @sph

I didn't test it, but the patch looks fine to me, except for the comment in the test: there, I'd recommend to just state that it tests enabling and disabling of warnings, and not mention _Optional at all; having the test not depend on _Optional is good, but stating that explicitly increases mental load of readers for those who don't know about _Optional yet.

Agreed and modified accordingly. [r17099] adds --enable-warning and #pragma enable_warning.

[Christopher Bazley]

Please review the first of two follow-up patches. The full feature seems too big to be easily digestible as a single patch. This first patch adds and documents a new warning for implicit conversions of explicitly supplied null pointer constants. Omitted initialisers do not generate the new warning, even where they necessarily result in null pointer values; those cases are covered by the second patch.

The most interesting part of this patch is the special treatment required for the conditional operator (?:). An explicit goal is to leave expression types unchanged. However, applying the warning solely according to the conditional expression's result type would produce spurious diagnostics.

For example:

_Optional int *foo(bool c, int *pi)
{
    return c ? pi : nullptr;
}

The conditional expression has type int *. A naive implementation would therefore warn about converting nullptr to that type, even though the expression is being returned as a pointer to an _Optional-qualified type, which permits a null value.

The patch instead checks null operands against the destination type, without changing the conditional expression's type.

(Replaced attached patch at 19:08 on 30 Oct)

[Christopher Bazley]

This second follow-up patch extends the warning to implicit null initialisation of omitted members and array elements, but only where an explicit initialiser is supplied, including in compound literals. Declarations without an initialiser do not trigger the warning.

For example, I think it would be very annoying for this declaration:

int main(void)
{
  BigObject bo;
  init_big_object(&bo);
  return 0;
}

to start generating a warning merely because bo is given static storage duration:

int main(void)
{
  static BigObject bo;
  init_big_object(&bo);
  return 0;
}

Without the exemption, the second declaration could warn about pointer members implicitly initialised to null, even though init_big_object subsequently assigns their intended values.

Such warnings would be especially troublesome where BigObject is defined in a library header: the programmer declaring the object cannot necessarily change its member types. Similarly, file-scope pointer objects initialised by later assignments would need either explicit initialisers or _Optional-qualified referenced types merely to avoid this warning.

These cases reflect a common pattern in C: storage is allocated first, and an initialisation function subsequently establishes the object's intended state. The language does not ensure that this function is called, but warning about the preceding zero-initialisation would penalise that pattern without establishing that the function is called correctly.

The proposed boundary is therefore the presence of an explicit initialiser. Supplying one enables warnings about omitted subobjects that are implicitly initialised to null; omitting it leaves subsequent initialisation to the programmer. This preserves warnings for incomplete aggregate initialisers without imposing them on every statically allocated object containing pointers.

[Maarten Brock]

Without having looked into the patch...

If init_big_object takes a const BigObject * it cannot initialize and the warning would be ok.

[Christopher Bazley]

Hi @maartenbrock

If init_big_object takes a const BigObject * it cannot initialize and the warning would be ok.

I think you mean that it would be okay to warn about bo if it were declared as static const with no initialiser. That is an excellent point that I had not previously considered.

It would still arguably be an encapsulation violation for warning-free compilation of a user program to depend on the member types of the declared object, if the type of that object were defined by a third-party library; on the other hand, I struggle to think of a use-case for an immutable object that can never be in a valid state.

If a third-party library accepts null values of members of pointer type as a valid state, their referenced types should be _Optional-qualified, regardless of whether that valid state comes from implicit initialisation or a call to a library function.

I've prepared a follow-up patch to implement your proposed extension as I understand it.

[Christopher Bazley]

I struggle to think of a use-case for an immutable object that can never be in a valid state.

This disregards the fact that a library can document that any complete struct type it defines in its headers can be implicitly initialised to a valid value. For example, a length-and-pointer string type:

// Empty string if implicitly initialised
struct string { size_t len; const char *buf; };

Such a type could then be instantiated and used thus:

#include "thirdpartystrings.h"
static const struct string empty;
int main()
{
  return string_get_len(&empty);
}

Warning about implicit initialisation of the buf member to null would break this use-case. A work-around would be to remove the const qualifier from the declaration (which is not necessarily acceptable) or wrap the declaration in a union:

static const union {
  char nil;
  struct string obj;
} empty;

If a third-party library accepts null values of members of pointer type as a valid state, their referenced types should be _Optional-qualified, regardless of whether that valid state comes from implicit initialisation or a call to a library function.

The above forgets that allowing use of third party libraries that don't qualify the referenced type of their pointers is a potential use-case for permitting initialisers to be omitted, but then such a workaround would not be applicable to immutable objects anyhow.

I also think the extra complexity of warning about declarations of objects with static storage duration and no initialiser needs to be weighed as part of the equation. Because of tentative definitions, it may not be known until the end of a translation unit whether or not such an object has an initialiser. (This is a general problem with warning about definitions at file scope that lack an initialiser — not something specific to const-qualified types.) In contrast, warnings relating to partial initialisers can be emitted as soon as the initialiser is encountered.