Skip to content

[clang][Sema] Suggest/Hint Standard Library Include File #146227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ Improvements to Clang's diagnostics
diagnostics. This fixes a bunch of `bool` being printed as `_Bool`, and also
a bunch of HLSL types being printed as their C++ equivalents.
- Clang now consistently quotes expressions in diagnostics.
- Clang now suggest standard library include path and its associated C++ or C language version.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Clang now suggest standard library include path and its associated C++ or C language version.
- Clang now suggests including standard library headers when encountering standard types.

- When printing types for diagnostics, clang now doesn't suppress the scopes of
template arguments contained within nested names.
- The ``-Wshift-bool`` warning has been added to warn about shifting a boolean. (#GH28334)
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -5990,6 +5990,10 @@ def err_template_expansion_into_fixed_list : Error<
"template|concept}0">;
def note_parameter_type : Note<
"parameter of type %0 is declared here">;
def note_standard_lib_include_suggestion : Note<
"maybe try to include %0; '%1' is defined in %0">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"maybe try to include %0; '%1' is defined in %0">;
"'%1' is defined in %0; did you mean to include it?">;

def note_standard_lib_version : Note<
"'%0' is a %1 feature">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably be doing something more like:
"'%0::%1' is a %enum_select..." for this.


// C++11 Variadic Templates
def err_template_param_pack_default_arg : Error<
Expand Down
10 changes: 10 additions & 0 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -3613,6 +3613,16 @@ class Sema final : public SemaBase {
ParsedType &SuggestedType,
bool IsTemplateName = false);

// Try to suggest the missing standard library include files
//
// \param SymbolName the symbol name like 'cout'
// \param SourceLocation the location of the note being emitted
// \param Namespace the namespace that must end with ::, so like 'std::'
void NoteStandardIncludes(StringRef SymbolName, SourceLocation IILoc,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan of this interface. Unless it is particularly easy, we shouldn't be forcing the inclusion of the '::' in this interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the interface in clangToolingInclusionsStdlib requires the inclusion of ::, so Namesace+SymbolName gives you the full name. I am fine with removing the inclusion of :: at the end.

StringRef Namespace);
void NoteStandardIncludes(StringRef SymbolName, SourceLocation IILoc,
const CXXScopeSpec *SS);

/// Attempt to behave like MSVC in situations where lookup of an unqualified
/// type name has failed in a dependent context. In these situations, we
/// automatically form a DependentTypeName that will retry lookup in a related
Expand Down
22 changes: 22 additions & 0 deletions clang/include/clang/Tooling/Inclusions/StandardLibrary.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,27 @@ class NamespaceDecl;
class DeclContext;
namespace tooling {
namespace stdlib {
enum Version {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have enums for standards versions elsewhere, I don't want us maintaining ANOTHER one. So I'm against this enum entirely.

Unknown,

// c++ versions
CPlusPlusStart,
CPlusPlus11,
CPlusPlus14,
CPlusPlus17,
CPlusPlus20,
CPlusPlus23,
CPlusPlus26,
CPlusPlusEnd,

// c version
CStart,
C99,
C11,
CEnd
};

llvm::StringRef GetAsString(Version Ver);

class Symbol;
enum class Lang { C = 0, CXX, LastValue = CXX };
Expand Down Expand Up @@ -85,6 +106,7 @@ class Symbol {
std::optional<Header> header() const;
// Some symbols may be provided by multiple headers.
llvm::SmallVector<Header> headers() const;
Version version() const;

private:
Symbol(unsigned ID, Lang Language) : ID(ID), Language(Language) {}
Expand Down
14 changes: 14 additions & 0 deletions clang/lib/Parse/ParseExprCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,11 @@ bool Parser::ParseOptionalCXXScopeSpecifier(
HasScopeSpecifier = true;
}

// If `FailedNestedNameBuilding` is true, attempt to suggest the standard
// include file corresponding to the current `FullNamespace` and symbol.
std::string FullNamespace = "";
bool FailedNesatedNameBuilding = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool FailedNesatedNameBuilding = false;
bool FailedNestedNameBuilding = false;


// Preferred type might change when parsing qualifiers, we need the original.
auto SavedType = PreferredType;
while (true) {
Expand Down Expand Up @@ -454,6 +459,9 @@ bool Parser::ParseOptionalCXXScopeSpecifier(
// We have an identifier followed by a '::'. Lookup this name
// as the name in a nested-name-specifier.
Token Identifier = Tok;
FullNamespace += Identifier.getIdentifierInfo()->getName();
FullNamespace += "::";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is extra work, just make it a part of the printing, not here. Better yet, instead of storing FullNameSpace, just store the IdentifierInfo list.


SourceLocation IdLoc = ConsumeToken();
assert(Tok.isOneOf(tok::coloncolon, tok::colon) &&
"NextToken() not working properly!");
Expand All @@ -465,6 +473,7 @@ bool Parser::ParseOptionalCXXScopeSpecifier(
if (Actions.ActOnCXXNestedNameSpecifier(
getCurScope(), IdInfo, EnteringContext, SS, CorrectionFlagPtr,
OnlyNamespace)) {
FailedNesatedNameBuilding = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we ONLY are going to want to suggest things of the pattern NS::TypeName. So ensuring we have hte 'full name' isn't really necessary. I think we can just store the namespace and identifier.

// Identifier is not recognized as a nested name, but we can have
// mistyped '::' instead of ':'.
if (CorrectionFlagPtr && IsCorrectedToColon) {
Expand Down Expand Up @@ -554,6 +563,11 @@ bool Parser::ParseOptionalCXXScopeSpecifier(
break;
}

if (FailedNesatedNameBuilding && Tok.getKind() == tok::identifier) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parser isn't really the right place to do this. This is very much a 'sema' diagnostic/suggestion. This should happen not here, but on failed lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completed agree with you for the fact that this is really ugly code here.

The problem I am trying to solve here is when there is no declaration for the std namespace.

// this is the entire file 
void some_func(){
     std::cout << "some func has been called" << std::endl; 
}

Here building the nested name will fail because there is no namespace std being defined anywhere, so a name lookup for cout is never performed (as far as I can tell, there could be something I am missing here). I am not sure if there is an easy fix to that in Sema level.

However, something like the following would work cause the suggestion to trigger. It is okay if I revert this file?

namespace std {};

// this is the entire file 
void some_func(){
     std::cout << "some func has been called" << std::endl; 
}

Actions.NoteStandardIncludes(Tok.getIdentifierInfo()->getName(),
Tok.getLocation(), FullNamespace);
}

// Even if we didn't see any pieces of a nested-name-specifier, we
// still check whether there is a tilde in this position, which
// indicates a potential pseudo-destructor.
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Sema/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,5 @@ add_clang_library(clangSema
clangEdit
clangLex
clangSupport
clangToolingInclusionsStdlib
)
50 changes: 50 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@
#include "clang/Sema/SemaSwift.h"
#include "clang/Sema/SemaWasm.h"
#include "clang/Sema/Template.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/STLForwardCompat.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/Frontend/HLSL/HLSLRootSignature.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/TargetParser/Triple.h"
#include <algorithm>
Expand Down Expand Up @@ -804,6 +807,52 @@ void Sema::DiagnoseUnknownTypeName(IdentifierInfo *&II,
assert(SS && SS->isInvalid() &&
"Invalid scope specifier has already been diagnosed");
}

// don't note standard include files for OpenCL and Objective C
if ((getLangOpts().CPlusPlus || getLangOpts().C99) && !getLangOpts().OpenCL &&
!getLangOpts().ObjC)
NoteStandardIncludes(II->getName(), IILoc, SS);
}

void Sema::NoteStandardIncludes(StringRef SymbolName, SourceLocation IILoc,
StringRef Namespace) {
using clang::tooling::stdlib::Lang;

llvm::StringRef HeaderName = "";
tooling::stdlib::Lang LangOption = tooling::stdlib::Lang::C;
if (getLangOpts().CPlusPlus)
LangOption = clang::tooling::stdlib::Lang::CXX;

if (auto StdSym =
tooling::stdlib::Symbol::named(Namespace, SymbolName, LangOption)) {
if (auto Header = StdSym->header()) {
HeaderName = Header->name();
Diag(IILoc, diag::note_standard_lib_include_suggestion)
<< HeaderName << (Namespace + SymbolName).str();

// Noting the C/C++ version as well
if (StdSym->version() != tooling::stdlib::Unknown) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version stuff we should be handling via our diagnostic, not doing it this way.

llvm::StringRef CPlusPlusVersion =
tooling::stdlib::GetAsString(StdSym->version());

Diag(IILoc, diag::note_standard_lib_version)
<< (Namespace + SymbolName).str() << CPlusPlusVersion;
}
}
}
}

void Sema::NoteStandardIncludes(StringRef SymbolName, SourceLocation IILoc,
const CXXScopeSpec *SS) {
std::string Namespace = "";
if (SS) {
llvm::raw_string_ostream Stream(Namespace);
if (SS->isValid())
SS->getScopeRep()->dump(Stream);
Stream.flush();
}

NoteStandardIncludes(SymbolName, IILoc, Namespace);
}

/// Determine whether the given result set contains either a type name
Expand Down Expand Up @@ -16783,6 +16832,7 @@ NamedDecl *Sema::ImplicitlyDefineFunction(SourceLocation Loc,
}

Diag(Loc, diag_id) << &II;
NoteStandardIncludes(II.getName(), Loc, "");
if (Corrected) {
// If the correction is going to suggest an implicitly defined function,
// skip the correction as not being a particularly good idea.
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2658,11 +2658,17 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
if (!SS.isEmpty()) {
Diag(R.getNameLoc(), diag::err_no_member)
<< Name << computeDeclContext(SS, false) << NameRange;
NoteStandardIncludes(Name.getAsString(), R.getNameLoc(), &SS);
return true;
}

// Give up, we can't recover.
Diag(R.getNameLoc(), diagnostic) << Name << NameRange;

// don't note standard include files for OpenCL and Objective C
if ((getLangOpts().CPlusPlus || getLangOpts().C99) && !getLangOpts().OpenCL &&
!getLangOpts().ObjC)
NoteStandardIncludes(Name.getAsString(), R.getNameLoc(), /*Namespace=*/"");
return true;
}

Expand Down
Loading
Loading