Page MenuHomePhabricator

Diagnosing the Future Keywords
ClosedPublic

Authored by Codesbyusman on Aug 11 2022, 7:39 AM.

Details

Summary

The patch diagnoses a Keyword as a future keyword if it exist in the upcoming language mode. such that

int restrict;

in the c modes earlier than C99 then will give a warning to the user that is a future keyword in C99. Now it handles keywords from C as well as C++

Diff Detail

Event Timeline

Codesbyusman created this revision.Aug 11 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 7:39 AM
Codesbyusman requested review of this revision.Aug 11 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 7:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 added inline comments.
clang/include/clang/Basic/TokenKinds.def
390–391

nullptr is now in C23, right?

Codesbyusman retitled this revision from "Diagnosing the Future Keywords" to Diagnosing the Future Keywords.Aug 11 2022, 7:44 AM
Codesbyusman edited the summary of this revision. (Show Details)
Codesbyusman added reviewers: erichkeane, xgupta.

I believe you need tests here desperately. I also believe the define logic that you have here isn't going to be sufficient for the C future flags thanks to it getting some C keywords.

clang/include/clang/Basic/TokenKinds.def
382

Hmm... this looks like it is going to be troublesome for the 'future' feature. Can you make sure you have tests for all of these?

clang/lib/Lex/Preprocessor.cpp
780

This isn't right at all. We should be looking through the list of flags instead of trying to assume that cxx11 keywords here are all 'future' C keywords. First, this isn't true. Second, even if it was, it is really fragile.

aaron.ballman added inline comments.Aug 11 2022, 7:54 AM
clang/include/clang/Basic/TokenKinds.def
390–391

It is, but that's separate from the rest of these changes (it would need testing and other work).

Codesbyusman added inline comments.Aug 11 2022, 7:58 AM
clang/include/clang/Basic/TokenKinds.def
382

Hmm... this looks like it is going to be troublesome for the 'future' feature. Can you make sure you have tests for all of these?

Yes working for the test cases

clang/lib/Lex/Preprocessor.cpp
780

This isn't right at all. We should be looking through the list of flags instead of trying to assume that cxx11 keywords here are all 'future' C keywords. First, this isn't true. Second, even if it was, it is really fragile

Yes I was looking to it, But not getting how to deal this.
Will need to make the different Case to access some of the keywords that are define in CXX11_KEYWORD.
Any suggestions?

erichkeane added inline comments.Aug 11 2022, 8:04 AM
clang/include/clang/Basic/TokenKinds.def
382

I meant, for tests you haven't written yet. You need to write tests to validate that the future diagnostics are emitted properly.

clang/lib/Lex/Preprocessor.cpp
780

I suspect this pattern of using the define over the C99_KEYWORD/etc is not going to be sufficient. You'll likely have to just define KEYWORD, use it to 'extract' the entire list of flags, then check to see which language flag is 'set' vs the current LangOpts. You might be able to replace the C++ part of this that way to unify the implementations.

So something like:

unsigned Flags = llvm::StringSwitch<unsigned>(II.getName())
#define KEYWORD(NAME, FLAGS) .Case(#NAME, FLAGS)
#include "clang/Basic/TokenKinds.def
;
// Then, do a bunch of checks of the current 'lang' version vs the 'set' flag of the keyword.
to268 added a subscriber: to268.Aug 14 2022, 4:42 AM
Codesbyusman edited the summary of this revision. (Show Details)

updated.

I didn't have time to dig into the correctness of the TokenKinds or test changes (hopefully @aaron.ballman can take some time on that), but implementation overall looks correct, just with some style/formatting issues.

clang/include/clang/Basic/TokenKinds.def
85

NiT: Would be nice to put these next to the OTHER language standards

clang/lib/Basic/IdentifierTable.cpp
844

This comment block shouldn't be here at all.

855

This comment doesn't really make any sense here. I dont understand what you're trying to say.

866

This comment isn't helpful.

876

just make this an 'else'.

Precommit CI is failing with a failure that looks like it's relevant:

******************** TEST 'Clang :: Parser/static_assert.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 2';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 3';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c2x -Wpre-c2x-compat -verify=c2x-compat C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 4';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c99 -verify=c99 C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 5';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 6';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c++17 -verify=cxx17 -x c++ C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 7';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 8';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c++98 -verify=cxx98 -x c++ C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 9';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
: 'RUN: at line 10';   c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -fsyntax-only -std=c++17 -Wpre-c++17-compat -verify=cxx17-compat -x c++ C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\ws\w9\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w9\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-fsyntax-only" "-std=c2x" "-DTEST_SPELLING" "-verify=c2x" "C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c"
# command stderr:
error: 'error' diagnostics expected but not seen: 
  File C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c Line 18: expected parameter declarator
  File C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c Line 18: expected ')'
  File C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c Line 18: a type specifier is required for all declarations
error: 'warning' diagnostics seen but not expected: 
  File C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c Line 18: use of 'static_assert' without inclusion of <assert.h> is a Microsoft extension
error: 'note' diagnostics expected but not seen: 
  File C:\ws\w9\llvm-project\premerge-checks\clang\test\Parser\static_assert.c Line 18: to match this '('
5 errors generated.

error: command failed with exit status: 1

--

********************

I think this is coming from the fact that you're marking a bunch of keywords as being C2x keywords. That's correct insomuch as they are C2x keywords. But it's incorrect because we haven't actually implemented those features yet in the rest of the compiler. I think we should continue to treat the C2x keywords as we did previously. For the C2x keywords in the test file, you can put a FIXME comment above the ones that should get a warning whenever we finish the support for that keyword. WDYT @erichkeane?

clang/include/clang/Basic/DiagnosticLexKinds.td
86

I think the diagnostic group here is incorrect -- this grouping is used for things which are not compatible with standards before C2x. I think you need to add a new group for C2xCompat to DiagnosticGroups.td and use it here.

clang/include/clang/Basic/IdentifierTable.h
672–674
clang/include/clang/Basic/TokenKinds.def
88–89

I think this should be C2X_KEYWORD and KEYC2X for the moment (C23 isn't published yet so we're sticking with its placeholder name elsewhere in the code base).

384

This should be KEYC23.

clang/lib/Basic/IdentifierTable.cpp
90–91

It's worth noting that there are no C11 keywords that we handle only in C11+ mode, so we could probably repurpose this value for a different language mode.

Codesbyusman marked 5 inline comments as done.

updated with suggestios

Looks about right, please see if you can do more work on the static-assert test to make sure we don't lose other testing behavior, AND see the other comment.

clang/lib/Lex/Preprocessor.cpp
847

Don't create a new one, do getIdentifierTable() to get the active identifier table.

clang/test/Parser/static_assert.c
18–24

So we want to make sure we still get the ms-warnings, so this also needs to be validated in C++17 mode as well, which should ALSO give us a 'future keyword' diagnostic.

aaron.ballman added inline comments.Aug 23 2022, 6:59 AM
clang/include/clang/Basic/DiagnosticLexKinds.td
85
clang/include/clang/Basic/TokenKinds.def
256
384

This is technically correct, but I think we should remove it until we go to implement that paper instead of introducing the keyword out of thin air here.

Btw, I think that should be , 0 instead of , KEYC2X given the use of C2X_KEYWORD, right?

clang/lib/Lex/Preprocessor.cpp
789–791

You should be sure to add this comment to the new code so we don't lose the useful information.

updated with suggestions and test cases.

updated test cases and suggestions.

Codesbyusman added inline comments.Aug 24 2022, 4:01 AM
clang/include/clang/Basic/TokenKinds.def
384

This is technically correct, but I think we should remove it until we go to implement that paper instead of introducing the keyword out of thin air here.

Btw, I think that should be , 0 instead of , KEYC2X given the use of C2X_KEYWORD, right?

Don't know much but what I understand that this is a keyword but not published officially by language but will be in future. so I think will be good to remove it or can add Fixme?

aaron.ballman added inline comments.Aug 24 2022, 11:07 AM
clang/include/clang/Basic/TokenKinds.def
384

Don't know much but what I understand that this is a keyword but not published officially by language but will be in future. so I think will be good to remove it or can add Fixme?

Close -- the issue is more that we have no support for remove_quals in the compiler and it's easier for someone to add the keyword when implementing that feature. So you should remove the line entirely -- someone else will add it in the future.

clang/lib/Basic/IdentifierTable.cpp
844

It looks like you might have missed this comment.

855

It looks like you might have missed this comment (it's about Checking the Language mode and then for the diagnostics.)

clang/test/Parser/static_assert.c
1–2

Why did you add -Weverything here? That seems odd.

23
Codesbyusman added inline comments.Aug 24 2022, 6:01 PM
clang/test/Parser/static_assert.c
1–2

Why did you add -Weverything here? That seems odd.

Totally unintentional. I was initially exploring something. will fix this. Thank you for pointing out.

I spotted one last (hopefully) concern, but otherwise this LG for functionality. Can you also add a release note for the improved diagnostic behavior?

clang/include/clang/Basic/TokenKinds.def
381–383

I'm very sorry for not noticing this until the last minute, but I think all three of these should be using KEYWORD instead of C2X_KEYWORD. We set the value for BOOLSUPPORT properly for C2x: https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/LangOptions.cpp#L196 so that should be a no-op change.

(Otherwise, it looks like these are C2x keywords but not C++ keywords, which seems rather strange)

aaron.ballman accepted this revision.Aug 26 2022, 6:14 AM

LGTM! There's a minor thing left to be fixed, but I'll handle it on commit. I wanted to drop the |KEYC2X for the Boolean keywords so it relies solely on BOOLSUPPORT, but then I realized we could just use the correct flags instead of using BOOLSUPPORT at all.

This revision is now accepted and ready to land.Aug 26 2022, 6:14 AM
This revision was landed with ongoing or failed builds.Aug 26 2022, 6:20 AM
This revision was automatically updated to reflect the committed changes.