Hi all,

The attached patch adds two messages inspired by C compiler's -Wswitch-enum and -Wcovered-switch-default. Building on the recent label count refactoring, this was fairly straightforward.

- If a case statement on an ordinal does not contain labels for all values of the ordinal, and no else statement is given, raise a new warning (W6059). This is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.

- If a case statement has labels for the entire range of the type, and an (now never used) else statement is present, a warning "Unreachable code" is raised.

Both cases are clearly something where the compiler and programmer don't agree on something, hence drawing attention to these situations.

The checks are enabled only for enumerated types and small (1-byte) integers. In C, they are only for enumerated types, I added small types because they are often used as tag types, where this check is extra useful.

Now, the RFC part. I have a few open questions...

* does it make sense to do that to all integral types? In ISO mode, it's formally required for all case statements. I left it out for now as sometimes case..of is used as a shorthand for multiple in..[range], there would be more detections that look like false positives.

* in ISO mode, there are 2 solutions: in Standard Pascal, not covering the entire type is a compile-time error. In Extended Pascal, this is a runtime error. Which one is followed usually?

* Building the compiler itself cycles with -Sew, so it bails on the first occurrence of one of these issues - turns out they're all over the place. However... I think the warnings are correct and expose imperfect code here. Take this example (one of the first encountered):
<https://github.com/graemeg/freepascal/blob/master/compiler/defutil.pas#L1630> stringtype is an enum and all values are used. There cannot be any other values (as we have established last year). Therefore, the else clause is just as wrong as an "if false then", which would be caught already.

* Adding to the previous, since it is now possible to discover forgotten items or later additions by the other warning, removing these 'safety' else-clauses would improve code quality.

What do you think?

Actually, in a perfect world you would use a suitable range type.

And how will I mage this when my code runs on different architecture where the full range of the ordinal may differ?

Which ordinal changes in FPC its range depending on the architecture?

AM Martok wrote

- If a case statement on an ordinal does not contain labels for all values of the ordinal, and no else statement is given, raise a new warning (W6059). This is actually defined as an error in ISO7185 and a dynamic-violation in IEC10206.

So now I will have to add a useless else statement for every case statement that uses e.g. integers, or rewrite them to if..then..else or if value in [...]?
And how will I mage this when my code runs on different architecture where the full range of the ordinal may differ?

Please no (to this part of the proposal).

Bart

Am Mi., 2. Jan. 2019, 09:44 hat Martok
geschrieben:

* in ISO mode, there are 2 solutions: in Standard Pascal, not covering the entire type is a compile-time error. In Extended Pascal, this is a runtime error. Which one is followed usually?

ISO and Extended Pascal are handled as two separate modes by the compiler. So handling it according to the mode would probably be advisable.

Regards,
Sven

+1 here or at least decrease severity from warning to some level that is not by default printed in Messages window ...

-Laco.

It does not make sense. Consider the following:

Type
   TMyClass = class
   Private
     function GetString(AIndex: Integer): string;
   Published
     Property MyString : String Index 1 Read GetString;
   end;
function TMyClass.GetString(AIndex: Integer): string;
begin
   case AIndex of
    1 : Result:=GenerateSomestringvalue;
   end;
end;

I don't think there should be errors or warnings.

Michael.

Am Mi., 2. Jan. 2019, 11:08 hat Bart geschrieben:

Warnings can always be disabled by {$warn NR off} or - I believe - a command line switch.
One could also decide to have it be a default off warning for the non ISO modes (FPC, Delphi, MacPas) like some of the string conversion warnings.

Regards,
Sven

The compiler is for speed reasons not compiled with $R+/$O+, so the safety else clause have its purpose, but they should throw an internal error instead. Further, the advantage of an IE instead of a normal RTE is, that the IEs are pretty unique so the source location of an IE can be identified very quickly, as the release compiler is not compiled with -gl.

AM Florian Klämpfl wrote

Which ordinal changes in FPC its range depending on the architecture?

Integer is not the same range on 16-bit platform IIRC?

I would strongly advise to have it off by default for all but the ISO modes.

You have perfectly valid code today (see the example of indexed properties) which would start generating exceptions, and you should not be forced to add a {$ } directive or an "else" clause to a case statement for this.

I agree this warning is very useful for enumerated types, but really not for integers. There are too many cases where it would be a false positive.

Michael.

I do hope you meant warnings, not exceptions.

I agree this warning is very useful for enumerated types, but really not for integers. There are too many cases where it would be a false positive.

I have a lot of code with enums, where these warnings will force me to do a lot of changes.

Mattias

Yes, sorry, typo :(

I can imagine it.

Maybe the warning should also depend on the range checking on/off switch.

Michael.

Bart wrote

So now I will have to add a useless else statement for every case statement that uses e.g. integers,

Well, as Jonas argues on fpc-pascal, you are supposed to add useless statements anyway, which the compiler will then 100% remove...

j/k.

In all seriousness though, that is why I didn't include all integers. But enums and small ordinals are often used as a "what is this data" tag, where it is usually an oversight if some value is not checked. I'm already not super happy this also applies to Char (a OS_8 ordinal), because it makes case-based regular language parsing quite noisy.

Good example.

Although... you *could* call GetString from some other class member function, and at that point Result would be undefined. I don't think the compiler can prove that?
Wait - why does that code not raise a 5033 "Function Result does not seem to be set"? Add a second property "Property MyOtherString : String Index 2 Read GetString;", and now its provably wrong. No warning. This seems wrong, considering what we just talked about on fpc-pascal.

Wait - why does that code not raise a 5033 "Function Result does not seem to be set"? Add a second property "Property MyOtherString : String Index 2 Read GetString;", and now its provably wrong. No warning. This seems wrong, considering what we just talked about on fpc-pascal.

Because there is no SetLength call in the function?

+1

If someone wants to deliberately encumber themselves with using those standards, this might seem like a great idea. In reality, not so much.

Ralf

---
This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

The compiler can prove that since the methods are private. It can see no explicit calls are generated for it.

Michael.

Florian Klämpfl wrote

The compiler is for speed reasons not compiled with $R+/$O+, so the safety else clause have its purpose, but they should throw an internal error instead.

And here we are at that other issue again. There is absolutely no guarantee the safety clause is even present in the compiled ppc. The improved jumptable generation actually makes it less likely! (the state of $R,$O doesn't change this)

So there's really 2 possible goals:
- safety against programmer error: a new stringtype is invented, and not considered here. The coverage warning will tell the programmer, just as an IE (maybe) would during testing -- it's a bug either way.
- safety against data error: def deserialized from damaged ppu? Undefined behaviour, the else clause may not be present in the compiler binary, since all legal values are handled. Which is exactly what the Unreachable Code warning would tell you.

This segment of the compiler is specifically the kind of scenario that these warnings are designed to catch, and also the reason why they are warnings, not hints. Code that someone wrote and language spec do not agree, and the compiler will do something that is *very likely* unintended.

Michael Van Canneyt wrote

The compiler can prove that since the methods are private. It can see no explicit calls are generated for it.

I thought so too, but even with added explicit calls, there is no warning. Plus, it could only reason for this class if it was strict private, otherwise, anyone in this module could access it.

Doesn't seem to be class related, even this very obvioulsy wrong segment
generates no warning:
    function GetString(AIndex: Integer): string;
    begin
       case AIndex of
        1 : Result:= 'abc';
       end;
    end;
    begin
      writeln(GetString(42));
    end.

Martok wrote

Wait - why does that code not raise a 5033 "Function Result does not seem to be set"? Add a second property "Property MyOtherString : String Index 2 Read GetString;", and now its provably wrong. No warning. This seems wrong, considering what we just talked about on fpc-pascal.

It should print a warning if you compile with -Oodfa (or -O3, which includes -Oodfa). Without data flow analysis, the compiler cannot check whether the result is set on all possible paths, only whether it is set on at least one path.

Jonas

When I said 'it can', I meant this is theoretically possible. I didn't mean to imply it actually does.

Michael.

Thanks. I have added these warnings to the compiler in r42047, and also the static/dynamic errors for Standard resp. Extended ISO Pascal.

By default, the warnings are enabled for
* enums
* boolean
* integer subrange types that don't span the entire range of their storage (e.g. 0..254, -128..0, ..., but not shortint/byte/smallint/word/...)

The warning can be enabled for all ordinal types with the new -CC command line option. Like any other warning, it can be disabled with the corresponding -vm option (-vm6060 in this case).

Jonas

Jonas Maebe wrote

Thanks. I have added these warnings to the compiler in r42047, and also the static/dynamic errors for Standard resp. Extended ISO Pascal.

Very nice.

One question about "C-style enumeration types"
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31

Do the holes between undefined enum values belong to the enumeration or not? Because:

1.) The FPC documentation states they do not. See
https://www.freepascal.org/docs-html/ref/refse12.html#QQ2-26-31 /"The Pred and Succ functions cannot be used on this kind of enumeration types. Trying to do this anyhow will result in a compiler error."/

2.) The Delphi documentation states they do. See
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types_with_Explicitly_Assigned_Ordinality /"An enumerated type is, in effect, a subrange whose lowest and highest values correspond to the lowest and highest ordinalities of the constants in the declaration."/

3.) Your newly added warnings indicate they do. Because a warning is emitted if all well-defined values are used in the case statement:

program Project1;
{$mode objfpc}
type
   TMyEnum = (one = 2, two = 5);
var
   A: TMyEnum;
begin
   A := TMyEnum(3);
   case A of // compiler warning: Case statement does not handle all
possible cases
     one: Exit;
     two: Exit;
   end;
   Writeln(A); // run-time error 107 - so maybe not a possible case
after all?
   ReadLn;
end.

---

This dissimilarities should get resolved.

Ondrej

It's because Pred and Succ do not make sense with enumerations with holes in our opinion, and could be confusing. E.g. in your example, one could expect at first sight that succ(one) = two.

I simply forgot to take them into account. I'm tempted to disable the warning altogether for them, because
1) Semantically, it would make sense to give a warning only if not all explicitly defined enum values are covered
2) However, you still can't give an "unreachable code" warning if there's an "else" in that case, because implicit values can also be stored in the enum

That would make the warnings inconsistent.

Jonas

It depends if the holes are valid enumeration values or not. If they are, Pred and Succ would make perfect sense - Delphi documentation explicitly states that Pred/Succ can and should be used to navigate to the holes:

"type Size = (Small = 5, Medium = 10, Large = Small + Medium); Only three of these values have names, but the others are accessible through typecasts and through routines such as Pred, Succ, Inc, and Dec." - see
http://docwiki.embarcadero.com/RADStudio/Rio/en/Simple_Types_(Delphi)#Enumerated_Types

But I am completely fine about your decision to disable Pred/Succ for C-style enumeration types (you did so only for non-delphi modes).

Yet, it still does not answer my question if the holes belong to the valid enumeration values from the FPC point-of-view or not.

Again, it all depends if the holes are valid enumeration values or not.

Your (1) suggests that they are not, whereas your (2) suggests they are.

Btw. subrange types have the same problem with implicit values - the same with (2):

program TestRanges;
{$mode objfpc}
type
   TMyRange = 2..5;
var
   R: TMyRange;
begin
   R := Default(TMyRange); // assign implicit value
   case R of
     2..5: Exit;
   else // compiler warning: unreachable code
     Writeln('1: ', R);// unreachable code is still executed despite the
warning!
   end;
   Writeln('2: ', R);
   ReadLn;
end.

Furthermore I strongly disagree with your (2) statement. On the contrary, if there are all explicitly defined cases covered and there is an else statement (like in the TestRanges program above), a warning about the unnecessary case statement is very needed because it is an "implementation detail" of the compiler if the else block will ever be executed or not (AFAIR you supported this). Someone in the future can decide that if the compiler finds an unnecessary else block, it will just ignore it. Don't call the warning "unreachable code" if you don't like it - call it whatever you want but keep it there.

Ondrej

Whether or not they are valid does not make it any less confusing.

If they are, Pred and Succ would make perfect sense - Delphi documentation explicitly states that Pred/Succ can and should be used to navigate to the holes:

I know, but I think that is not obvious from the declaration.

Yet, it still does not answer my question if the holes belong to the valid enumeration values from the FPC point-of-view or not.

As far as range checking and undefined behaviour is concerned, they do. I.e., you won't get undefined behaviour by assigning any value between low(enum)..high(enum) to them.

My (1) simply means that if you declare an enum with three explicit values, those are probably the only ones you'll want to explicitly handle in a case-statement. But yes, you'll probably still want an else-statement to catch errors in case another value got in there somehow, so I'll leave the warning as it is now.

Btw. subrange types have the same problem with implicit values - the same with (2):

Subrange types don't have "implicit values". You have values that are valid/in the range, and you have values that are invalid/out of range. There is nothing in between, unlike with enumeration types with holes.

The bug here is that this compiles even when range checking is enabled. This stores an invalid value in R, hence anything that uses R after this statement has undefined behaviour. The compiler should warn about that without range checking, and given an error with range checking (like fore other range checking errors). An alternative is for Default() to return a different value than 0 in this case, but I don't know if that would fix more than it breaks.

The TestRanges program is not about an implementation detail, but about undefined behaviour. Those are two completely different things.

Someone in the future can decide that if the compiler finds an unnecessary else block, it will just ignore it. Don't call the warning "unreachable code" if you don't like it - call it whatever you want but keep it there.

The compiler will always warn before removing unreachable code and will call it as such.

Jonas

Jonas Maebe wrote

As far as range checking and undefined behaviour is concerned, they do. I.e., you won't get undefined behaviour by assigning any value between low(enum)..high(enum) to them.

Very good, thank you. IMO this should be clearly documented to avoid confusion.

And if they are valid they should not cause any runtime errors:

program Project1;
{$mode objfpc}
type
   TMyEnum = (two=2, five=5);
var
   E: TMyEnum;
begin
   E := TMyEnum(3);
   Writeln(E); // RunError (107)
end.

(From the above example one would say they are not valid.)

There was a misunderstanding. I thought you meant with "implicit" values the "implicit default values" (=0):

program Project1;
{$mode objfpc}
type
   TMyEnum = (two=2, five=5);
   TMyObject = class
     E: TMyEnum;
   end;
var
   O: TMyObject;
begin
   O := TMyObject.Create;
   Writeln(Ord(O.E)); // R has an implicit default valuethat is invalid
   ReadLn;
end.

But you meant the implicit valid values (the holes). Now it's clear.

Best
Ondrej

True.

write(enum) is an FPC extension. It writes out the declared enum identifier. There is none in this case, so a run time error is appropriate.

Ah, indeed.

Jonas

Hi,

don't get me wrong, I like having something of mine included, but I'm genuinely surprised you used it in the way I designed it.

As you said, the warning and code that is actually generated must agree, otherwise they're useless. And there is a huge gap in the definition of the Freepascal interpretation of what "all values of the type" even means, with the code-as-written sometimes using one interpretation and sometimes the other.

On the topic of enums with assigned ordinals:

I simply forgot to take them into account. I'm tempted to disable the warning altogether for them, because

Please don't. Having sanity checks for "easy" types (where you don't really need them) and not having them for "complicated" types (where they would likely catch programmer errors) is not super helpful, and I would consider this as *worse* than the status quo of not having warnings at all, because it would give a false sense of safety ("My code compiles with -Sew? I must have done everything right!"). I'll never understand why Borland took that short cut for enumeration typeinfo and got away with it.

1) Semantically, it would make sense to give a warning only if not all explicitly defined enum values are covered

Agreed.

2) However, you still can't give an "unreachable code" warning if there's an "else" in that case, because implicit values can also be stored in the enum

Which takes us aaaaaall the way back to the original problem.

In my (and Borlands, and Microsofts) opinion, this program should compile without raising the two new warnings (all named values are used AND else for unnamed values 3 and 4), rangecheck as correct (3 in [2..5]), and be fully should show that the else branch is always taken, which may then mark the case labels as dead code.

program Project1;
{$mode objfpc}
type
  TMyEnum = (one = 2, two = 5);
var
  A: TMyEnum;
begin
  A := TMyEnum(3);
  case A of
    one: Exit;
    two: Exit;
  else
    Writeln('Other');
  end;
end.

With R := Default(TMyRange), which assigns 0 [side note: why is the default value an invalid value? That makes no sense at all], I would expect the same results except for a range check warning at compile time (Range check error while evaluating constants (0 must be between 2 and 5)).

I would *wish for* (and Borland documented this) the code to still be runtime-safe even in $R- in this case, but I'm well aware that you and Florian have already declined that. For completeness: subrange types silently upgrade to their storage types if required, which to me implies also in comparisons, which implies the "none of the above" label must always mean the entire storage type.

I'm still in favour of making an effort to formally define at least the core elements of the Freepascal languages (syntax, statements, types, overloads, resolution order, typeconv order, ...) with as few implementation quirks as possible and then align the compiler to it (not the other way around). Having less corner cases to juggle in one's mind may make things easier, and a formal spec is also a good reference for future extensions to see if ideas are orthogonal or not. But that's a whole different topic.

I do like this new warning.

I just don't understand why (for the else-block) it is shown at the end of the first statement, just before the semicolon (see the | where the warning is shown):

program Project1;
type
   TMyEnum = (one, two);
   procedure A;
   begin
   end;
var
   E: TMyEnum;
   I: Integer;
begin
   case E of
     one: ;
     two: ;
   else
     I := 15|;
     A;
   end;
end.

IMO it would make much more sense if it was shown in front of or behind the else keyword:

|else
else|

Ondrej