Page MenuHomePhabricator

Fix for Bug 8446. Template instantiation without a 'typename' keyword.
ClosedPublic

Authored by zahiraam on Jan 11 2018, 7:36 AM.

Diff Detail

Repository
rC Clang

Event Timeline

zahiraam created this revision.Jan 11 2018, 7:36 AM
rnk requested changes to this revision.Jan 17 2018, 1:51 PM
rnk added a reviewer: rsmith.

From the changes to the test suite, I can't see why we'd want to do this. This greatly reduces our ability to accept but warn on invalid code.

test/SemaTemplate/ms-lookup-template-base-classes.cpp
209 ↗(On Diff #129451)

All of these new errors are accepted by MSVC. This seems like a step backwards. The most common kind of reference into a dependent base is probably a reference to a field from a base class, i.e. missing this->.

test/SemaTemplate/typename-specifier.cpp
166 ↗(On Diff #129451)

This seems bad, we're no longer warning people about non-conforming code.

This revision now requires changes to proceed.Jan 17 2018, 1:51 PM
zahiraam updated this revision to Diff 131057.Jan 23 2018, 6:36 AM
zahiraam updated this revision to Diff 131058.
zahiraam marked an inline comment as done.
rnk added inline comments.Feb 6 2018, 12:12 PM
test/SemaTemplate/lookup-dependent-bases.cpp
16 ↗(On Diff #131058)

Making this an error seems like a compatibility regression, here and elsewhere.

zahiraam updated this revision to Diff 134934.Feb 19 2018, 9:02 AM
zahiraam marked an inline comment as done.
erichkeane added inline comments.Feb 28 2018, 7:47 AM
lib/Parse/ParseDecl.cpp
2619 ↗(On Diff #134934)

I believe we'd only want to do this in MSVCCompat mode, allowing invalid code in all situations is probably a bad idea.

2621 ↗(On Diff #134934)

Shouldn't SS also be checked to make sure its valid?

2623 ↗(On Diff #134934)

Does this correctly handle Template-templates?

2624 ↗(On Diff #134934)

What about RValues?

lib/Parse/ParseTentative.cpp
1513

Again, perhaps needs to handle RValues.

rnk added inline comments.Feb 28 2018, 5:10 PM
lib/Parse/ParseDecl.cpp
2619 ↗(On Diff #134934)

right

test/SemaTemplate/missing-typename.cpp
14

I'm guessing this is where the amp and star token type classification heuristic fires.

We should have a test case that goes the opposite direction, what happens if it's not a type name, but an expression? As in:

int g(int);
template <typename T>
int f(int x) {
  return g((T::InnerName & x) & x);
}
struct A { static const int InnerName = 42; };
int main() {
  f<A>(0);
}
zahiraam updated this revision to Diff 136732.Mar 2 2018, 6:26 AM
zahiraam marked 6 inline comments as done.

Hopefully I have responded to the issues you have raised.
I have added the test case you have submitted, to make sure that an expression is generated.
Some of the error messages in the lit test have to be updated (they do match MSVC error messages).

test/SemaTemplate/missing-typename.cpp
14

Added the test case.

rnk added inline comments.Mar 7 2018, 5:31 PM
lib/Parse/ParseDecl.cpp
2621 ↗(On Diff #136732)

This doesn't seem like the right spot to inject the MS compat check, shouldn't it be associated with the & and * token lookaheads?

2625 ↗(On Diff #136732)

Doing lookahead for identifiers doesn't seem like the right approach either. It could be an arbitrary expression, 0, a parenthetical expression, etc.

test/SemaCXX/invalid-member-expr.cpp
56 ↗(On Diff #136732)

This recovery seems worse, and this isn't an MS compat test, so this shouldn't change at all.

test/SemaCXX/typo-correction.cpp
527 ↗(On Diff #136732)

ditto

zahiraam updated this revision to Diff 141611.Apr 9 2018, 4:12 AM
zahiraam marked 3 inline comments as done.
rnk added inline comments.Apr 10 2018, 2:49 PM
lib/Parse/ParseDecl.cpp
2643 ↗(On Diff #141611)

The MSVC check should come first so that we short-circuit and so that readers know that this logic is only important for MS compatibility insanity.

Please also break this boolean condition up. Something like:

bool IsTemplateName = getLangOpts().CPlusPlus && NextToken().is(tok::less);
if (getLangOpts().MSVCCompat) {
  // <comments about patterns to match and avoid>
  IsTemplateName |= ...;
}
lib/Sema/SemaDecl.cpp
754 ↗(On Diff #141611)

Please make this issue an ext_typename_missing diagnostic in MS compat mode.

zahiraam updated this revision to Diff 142030.Apr 11 2018, 9:45 AM
zahiraam marked an inline comment as done.
lib/Parse/ParseDecl.cpp
2621 ↗(On Diff #136732)

Changing place of the condition about the MSVCCompat had the effect that the lit tests below passed without editing them. Is that acceptable?

2625 ↗(On Diff #136732)

Not sure about this. Could you provide an example?
Meanwhile I am positing a new patch.

lib/Sema/SemaDecl.cpp
754 ↗(On Diff #141611)

Not sure what you mean here?
DiagID is already set to ext_typename_missing for MSVC above. Or do you mean that the the condition to throw a diag should be (!IsTemplateName && MSVCCompat)?
Thanks.

rnk added inline comments.Apr 11 2018, 10:02 AM
lib/Parse/ParseDecl.cpp
2621 ↗(On Diff #136732)

Yes, the change is for readability, not functionality.

2625 ↗(On Diff #136732)

Sure, I'm suggesting something like my previous example:

int g(int);
template <typename T>
int f(int x) {
  return g((T::InnerName & x) & x);
}
struct A { static const int InnerName = 42; };
int main() {
  f<A>(0);
}

First, I don't see this as a test case below anywhere.

Second, I think you can replace x in (T::InnerName & x) with other integer expressions that don't start with an identifier:

return g((T::InnerName & 3) & x);
return g((T::InnerName & (3)));
return g((T::InnerName * 3) & x);

We need to accept these examples and instantiate T::InnerName & 3 as a binary operator expression, not a type name.

lib/Sema/SemaDecl.cpp
754 ↗(On Diff #141611)

I'm not sure what the correct condition is, but in the test cases you've added, we should be emitting a warning that we expected typename before the type names in place of the error that we emit without MSVC compat enabled.

zahiraam updated this revision to Diff 142418.Apr 13 2018, 8:13 AM
zahiraam marked 3 inline comments as done.
Godin added a subscriber: Godin.Jun 22 2018, 6:01 AM

Hello.

Feedback on this please. Thanks.

Hello,

Feedback please. Thanks.

rnk added a comment.Feb 7 2019, 10:53 AM

Broadly, what I think needs to happen is that we need to lift these compatibility checks before typo correction. See also this bug about typo correction interfering with our compatibility heuristics: https://bugs.llvm.org/show_bug.cgi?id=20249

What I see in this change is that you are setting IsTemplateName to true to arrange to reach the isMicrosoftMissingTypename codepath in DiagnoseUnknownTypeName, but we really should be moving the missing typename check before typo correction, or adding a separate parameter to DiagnoseUnknownTypeName to disambiguate the cases we're recovering from.

Lastly, sorry for the delay in response. My only excuse is that this parser code is hard to understand, so I tend to avoid looking at it. =?

lib/Parse/ParseDecl.cpp
2676 ↗(On Diff #142418)

I'm concerned that you set IsTemplateName to true above, but this code shouldn't execute, because there won't be any < > tokens after this name. In practice, if recovery succeeds, this code is never executed because we return earlier, but this doesn't seem well-structured.

lib/Parse/ParseTentative.cpp
1514–1515

I think I see how this works now, this is some tentative parsing logic that looks for reference or pointer type tokens (*, &, or &&) followed by > or ), which typically indicate that the punctuation is used as part of a type name.

lib/Sema/SemaDecl.cpp
656 ↗(On Diff #142418)

I'm concerned that this code is becoming harder to understand. In ParseDecl, you are setting IsTemplateName to true when an unknown type name happens to be followed by tokens that look like they indicate a pointer or reference type is being used. But, it's not really a template name. We are trying to recover in the situation where the user has named a non-template type inside of another dependent type. Then, we use IsTemplateName inside this function in ways that only really make sense if we're naming a template.

test/SemaTemplate/missing-typename.cpp
76

Thanks for adding these test cases, it gives me more confidence that we're on the right path.

zahiraam updated this revision to Diff 186837.Feb 14 2019, 6:54 AM
zahiraam marked 2 inline comments as done.
zahiraam added inline comments.Feb 14 2019, 6:54 AM
lib/Sema/SemaDecl.cpp
656 ↗(On Diff #142418)

I can't remember why I have added that. May be the code base has changed since I made this change.
The setting of IsTemplateName is definitely not needed. So this code is not in the new patch that I am uploading.

rnk added inline comments.Feb 14 2019, 2:12 PM
lib/Parse/ParseTentative.cpp
1513–1516

There are no checks here for -fms-compatibility, so this seems like it affects clang's behavior everywhere. Now we assume that (T::name *) is a type in all modes. Is that a problem? My best guess for what's supposed to happen here is that we're supposed to go down the HasMissingTypename codepath above to warn and suggest, hey, this can't possibly be an expression because of this token peeking heuristic, you should add typename.

Also, the indentation is off in phabricator. Please make sure there aren't any tabs when you upload the diff.

lib/Sema/SemaDecl.cpp
749 ↗(On Diff #186837)

The effect of this line is, if we're not in ms compat mode, and it is a template name, then don't emit the error. Is that correct?

zahiraam updated this revision to Diff 187580.Feb 20 2019, 7:13 AM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
lib/Parse/ParseTentative.cpp
1513–1516

Let's see if this this is acceptable to you. I didn't add a warning here since it will be generated later on. And considering the role of the current function.
Hopefully indentation is correct now.

rnk accepted this revision.Feb 20 2019, 2:24 PM

lgtm

This revision is now accepted and ready to land.Feb 20 2019, 2:24 PM
In D41950#1404899, @rnk wrote:

lgtm

I don't think I have commit right can you please commit it. Thanks.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 6:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Feb 25 2019, 6:23 PM

I don't think I have commit right can you please commit it. Thanks.

Sure, done!