Type::isIncompleteType()...

Steve_Naroff · February 11, 2008, 2:35am

Folks,

Do you think it makes sense for Type::isIncompleteType() to return true when the Tag definition is invalid?

For example...

   case Tagged:
      // A tagged type (struct/union/enum/class) is incomplete if the decl is a
      // forward declaration, but not a full definition (C99 6.2.5p22).
- return !cast<TagType>(CanonicalType)->getDecl()->isDefinition();
+ TagDecl *TD = cast<TagType>(CanonicalType)->getDecl();
+ return !TD->isDefinition() || TD->isInvalidDecl();

Since this predicate is fairly low-level, I wanted to get some feedback...

snaroff

Steve_Naroff · February 11, 2008, 4:07am

Folks,

Do you think it makes sense for Type::isIncompleteType() to return
true when the Tag definition is invalid?

I guess so. From the standard (6.2.5):

"… incomplete types (types that describe objects but lack information needed to determine their sizes). "

I guess it depends on what clients expect. Another way to look at this is that a bad tag definition doesn’t really type check at all; it’s a bogus definition, and doesn’t define any kind of type, including an incomplete one. It’s probably safer to have clients believe that a bad tag definition has an incomplete type, and then check why it is an incomplete type by interrogating the TagDecl.

I tend to agree with you. If we make this change, the code below will result in 2 errors. Note that both EDG & GCC only produce 1 error (the first one). From my perspective, not flagging the 2nd error is incorrect (though it could lead to some noisy diagnostics, I guess).

Neil, does your cfe produce 1 or 2 diagnostics for the following test case?

snaroff

[steve-naroffs-imac:tools/clang/test] snaroff% cat xx.c
struct S {
int a;
int foo();
};

struct S s;

[steve-naroffs-imac:tools/clang/test] snaroff% …/…/…/Debug/bin/clang xx.c
xx.c:3:7: error: field ‘foo’ declared as a function
int foo();
^
xx.c:6:10: error: variable has incomplete type ‘struct S’
struct S s;
^
2 diagnostics generated.

Steve_Naroff · February 11, 2008, 4:22am

The spec seems to define completeness by the closing brace…

6.7.2.3p3 All declarations of structure, union, or enumerated types that have the same scope and
use the same tag declare the same type. The type is incomplete109) until the closing brace
of the list deﬁning the content, and complete thereafter.

If so, changing the predicate doesn’t sound correct (though it makes sense to me conceptually).

snaroff

Eli_Friedman1 · February 11, 2008, 4:26am

The spec doesn't have anything to say about programs that don't meet
the constraints... so I don't think we need to worry about that.

-Eli

Steve_Naroff · February 11, 2008, 4:52am

The spec seems to define completeness by the closing brace...

If so, changing the predicate doesn't sound correct (though it makes sense
to me conceptually).

The spec doesn't have anything to say about programs that don't meet
the constraints... so I don't think we need to worry about that.

That's why I think this is an interesting issue...

I'm proposing modifying a spec defined constraint (isIncompleteType) to include a new "constraint" regarding not meeting constraints:-)

It sounds like you think it's the right thing to do...true?

snaroff

_Neil_Booth · February 11, 2008, 5:12am

Steve Naroff wrote:-

I tend to agree with you. If we make this change, the code below will
result in 2 errors. Note that both EDG & GCC only produce 1 error (the
first one). From my perspective, not flagging the 2nd error is
incorrect (though it could lead to some noisy diagnostics, I guess).

Neil, does your cfe produce 1 or 2 diagnostics for the following test
case?

snaroff

[steve-naroffs-imac:tools/clang/test] snaroff% cat xx.c
struct S {
  int a;
  int foo();
};

struct S s;

[steve-naroffs-imac:tools/clang/test] snaroff% ../../../Debug/bin/
clang xx.c
xx.c:3:7: error: field 'foo' declared as a function
  int foo();
      ^
xx.c:6:10: error: variable has incomplete type 'struct S'
struct S s;
         ^
2 diagnostics generated.

$ ./cfe /tmp/bug.c
"/tmp/bug.c", line 3: error: members may not have function type
int foo();
^

1 error found compiling "/tmp/bug.c".

I don't think it necessarily makes sense to expect extra diagnostics
in any particular situation; once something is erroneous there are
many reasonable recovery strategies. I think my cfe just ignores
the foo declaration as if it didn't exist, for example. Another
quite reasonable strategy would be to have a member foo in the
structure but flag it erroneous; this would mean uses of foo wouldn't
get diagnostics about no such member.

I'm not sure what the background is in this particular case.

Neil.

Chris_Lattner · February 11, 2008, 7:01am

In this specific case (a single malformed field in a struct) I agree with Neil: it's best to just recover by dropping that one field decl, or change it into a decl with a valid type (in the case of function, make it a pointer to function) which is probably even better. In certain parts of type analysis we try really hard to turn an erroneous type into a valid one.

As everyone knows, there is a tradeoff here: the more you try to "infer" what the user meant, the more likely it is that your guess can trigger a cascade of errors. In this specific case, I think that turning a function into a pointer to function is unlikely to do that though.

That said, Steve's idea might still be a good one. In what other cases can a struct decl be marked erroneous?

-Chris

_Neil_Booth · February 11, 2008, 9:49am

Chris Lattner wrote:-

>I don't think it necessarily makes sense to expect extra diagnostics
>in any particular situation; once something is erroneous there are
>many reasonable recovery strategies. I think my cfe just ignores
>the foo declaration as if it didn't exist, for example. Another
>quite reasonable strategy would be to have a member foo in the
>structure but flag it erroneous; this would mean uses of foo wouldn't
>get diagnostics about no such member.

In this specific case (a single malformed field in a struct) I agree
with Neil: it's best to just recover by dropping that one field decl,
or change it into a decl with a valid type (in the case of function,
make it a pointer to function) which is probably even better. In
certain parts of type analysis we try really hard to turn an erroneous
type into a valid one.

As everyone knows, there is a tradeoff here: the more you try to
"infer" what the user meant, the more likely it is that your guess can
trigger a cascade of errors. In this specific case, I think that
turning a function into a pointer to function is unlikely to do that
though.

That said, Steve's idea might still be a good one. In what other
cases can a struct decl be marked erroneous?

Actually my cfe does what I said second above: it has foo as a
member that is flagged erroneous. I put a lot of effort into
"clean" recovery; as far as possible I wanted something erroneous
to only cause a single complaint. So for example, compiling

struct S {
int a;
int foo();
};

struct S s;

void bar (void) { s.bar = s.foo = &s.a; }

gives the following:

neil@duron:~$ ~/src/cfe/cfe /tmp/bug.c
"/tmp/bug.c", line 3: error: members may not have function type
  int foo();
      ^
"/tmp/bug.c", line 8: error: structure "S" has no member "bar"
void bar (void) { s.bar = s.foo = &s.a; }
                    ^

2 errors found compiling "/tmp/bug.c".

Here you see that it knows bar is not a member and that foo is a
member, and that is doesn't complain about the type of the assignment
to foo (or bar) because foo's type is erroneous.

So rather than give an invalid construct a valid type, I remember
it as erroneous and suppress further diagnostics. With a bit of
effort this can be done in a non-intrusive way, and I find it slightly
preferable to giving a valid type that can cause further diagnostics.

Neil.

Devang_Patel1 · February 11, 2008, 5:47pm

$ ./cfe /tmp/bug.c
"/tmp/bug.c", line 3: error: members may not have function type
int foo();
^

1 error found compiling "/tmp/bug.c".

I don't think it necessarily makes sense to expect extra diagnostics
in any particular situation; once something is erroneous there are
many reasonable recovery strategies. I think my cfe just ignores
the foo declaration as if it didn't exist, for example. Another
quite reasonable strategy would be to have a member foo in the
structure but flag it erroneous; this would mean uses of foo wouldn't
get diagnostics about no such member.

In this specific case (a single malformed field in a struct) I agree
with Neil: it's best to just recover by dropping that one field decl,
or change it into a decl with a valid type (in the case of function,
make it a pointer to function) which is probably even better. In
certain parts of type analysis we try really hard to turn an erroneous
type into a valid one.

As everyone knows, there is a tradeoff here: the more you try to
"infer" what the user meant, the more likely it is that your guess can
trigger a cascade of errors.

.. which is not very useful to user. Yes, Steve's idea of reporting two error is good in this example. Even better solution is to clearly state that 2nd error is dependent on 1st error.

Chris_Lattner · February 11, 2008, 5:49pm

Right, I'm sorry I wasn't clear, but that's what I meant. In clang we try really hard to never create invalid ASTs, even for invalid code. To mark the field decl invalid, we first have to force the type to something valid, create the decl, then mark the decl as invalid. I completely agree with you that this is the right thing to do in this case.

-Chris