Skip to content
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

Partial fix for #11797: If sourcefile in imported project is missing then report critical error fileNotFound #5498

Closed
wants to merge 3 commits into from
Closed
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
16 changes: 16 additions & 0 deletions lib/cppcheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -659,8 +659,23 @@ unsigned int CppCheck::check(const std::string &path, const std::string &content
return checkFile(Path::simplifyPath(path), emptyString, &iss);
}

void CppCheck::fileNotFoundError(const std::string& filename)
{
mErrorLogger.reportErr(ErrorMessage({ErrorMessage::FileLocation(filename, 0, 0)},
filename,
Severity::error,
"File not found",
"fileNotFound",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error might be too generic. Something more specific would be better since there might be more errors like this coming soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The error might be too generic. Something more specific would be better since there might be more errors like this coming soon.

Certainty::normal));
mExitCode = 1;
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 not arbitrary set the exitcode. Severrity::error will already do this based on --error-exitcode. This will cause the analysis to fail even if the user suppresses this without any indication why.

}

unsigned int CppCheck::check(const ImportProject::FileSettings &fs)
{
if (!Path::isFile(fs.filename)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other projects perform these checks with importproject.cpp so this duplicates logic. Having an controllable logic might be better though. But we should implement and this properly this properly for all projects. We should not introduce more half-baked or inconsistent behavior. We still trying to get rid of such things from years and years ago.

I also think this might be the wrong location to do this. I think this should be reported closer to the actual import but I think we might be lacking common code to place this in.

Copy link
Owner Author

@danmar danmar Sep 29, 2023

Choose a reason for hiding this comment

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

We can put it in importproject also. But I wanted to achieve that the analysis will continue on other files that are found. I saw that if a file is missing when compile_commands.json is loaded then all analysis is aborted and that might be unfortunate. And putting this check here means that if the file has been removed after the project was loaded then we still detect this problem and bails out with an error message. Still.. maybe we can tune this and report it if simplecpp fails to read the source file instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understood the intention. I just want it to be consistent in implementation and test.

We should do in in the import step as we do with Path::acceptFile() so we know this as early as possible and not bail out on the first error. Similar to the changes I did for library and addon loading.

The problem is that we are not able to collect errors there - yet. That's what I added CmdLineLogger for. I want to collect all errors and report them with a proper ID after all options have been processed. But I need a few more things merged and changed first.

So this change is fine for now as we cannot solve it in a better way yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will post a draft PR soon after my pending stuff has been merged and I finished up most of the executor stuff. If things go well that should be as soon as next week.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I just want it to be consistent in implementation and test.

hmm yes

I will post a draft PR soon after my pending stuff has been merged and I finished up most of the executor stuff.

ok sure.

If things go well that should be as soon as next week.

I would like to have this in next major release 2.13 so well it's not a hurry yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just want it to be consistent in implementation and test.

hmm yes

Simply adding tests with missing files for all projects would be sufficient.

fileNotFoundError(fs.filename);
return mExitCode;
}
CppCheck temp(mErrorLogger, mUseGlobalSuppressions, mExecuteCommand);
temp.mSettings = mSettings;
if (!temp.mSettings.userDefines.empty())
Expand Down Expand Up @@ -1715,6 +1730,7 @@ void CppCheck::getErrorMessages(ErrorLogger &errorlogger)
cppcheck.purgedConfigurationMessage(emptyString,emptyString);
cppcheck.mTooManyConfigs = true;
cppcheck.tooManyConfigsError(emptyString,0U);
cppcheck.fileNotFoundError("missing.c");
// TODO: add functions to get remaining error messages

// call all "getErrorMessages" in all registered Check classes
Expand Down
3 changes: 3 additions & 0 deletions lib/cppcheck.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ class CPPCHECKLIB CppCheck : ErrorLogger {
/** @brief There has been an internal error => Report information message */
void internalError(const std::string &filename, const std::string &msg);

/** If check is called with a file that does not exist, this critical error message should be reported */
void fileNotFoundError(const std::string& filename);

/**
* @brief Check a file using stream
* @param filename file name
Expand Down
1 change: 1 addition & 0 deletions lib/errorlogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
const std::set<std::string> ErrorLogger::mCriticalErrorIds{
"cppcheckError",
"cppcheckLimit",
"fileNotFound",
"internalAstError",
"instantiationError",
"internalError",
Expand Down
10 changes: 10 additions & 0 deletions test/cli/test-proj2.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,13 @@ def test_gui_project_loads_absolute_vs_solution_2():
ret, stdout, stderr = cppcheck(['--project=test.cppcheck'])
assert ret == 0, stdout
assert stderr == ERR_A + ERR_B

def test_vs_project_missing_file():
with open('proj2/proj2.vcxproj', 'rt') as fin:
s = fin.read()
with open('proj2/test.vcxproj', 'wt') as fout:
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 use tmpdir if we are generating new files but none of the tests here currently do that.

fout.write(s.replace('a.c', 'a_missing.c'))
ret, stdout, stderr = cppcheck(['--project=proj2/test.vcxproj', '--file-filter=*/a*'], status_report=True)
assert 'proj2/a/a_missing.c:0:0: error: File not found [fileNotFound]' in stderr
assert 'There was critical errors' in stdout
os.remove('proj2/test.vcxproj')
4 changes: 2 additions & 2 deletions test/cli/testutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def lookup_cppcheck_exe():


# Run Cppcheck with args
def cppcheck(args, env=None):
def cppcheck(args, env=None, status_report=False):
exe = lookup_cppcheck_exe()
assert exe is not None, 'no cppcheck binary found'

Expand All @@ -72,6 +72,6 @@ def cppcheck(args, env=None):
comm = p.communicate()
stdout = comm[0].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
stderr = comm[1].decode(encoding='utf-8', errors='ignore').replace('\r\n', '\n')
if stdout.find('\nActive checkers:') > 0:
if (not status_report) and stdout.find('\nActive checkers:') > 0:
stdout = stdout[:1 + stdout.find('\nActive checkers:')]
return p.returncode, stdout, stderr
9 changes: 9 additions & 0 deletions test/testcppcheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class TestCppcheck : public TestFixture {

void run() override {
TEST_CASE(getErrorMessages);
TEST_CASE(testMissingSourceFile);
}

void getErrorMessages() const {
Expand Down Expand Up @@ -76,6 +77,14 @@ class TestCppcheck : public TestFixture {
ASSERT(foundPurgedConfiguration);
ASSERT(foundTooManyConfigs);
}

void testMissingSourceFile() {
CppCheck cppcheck(*this, false, nullptr);
ImportProject::FileSettings fileSettings;
fileSettings.filename = "missing.cpp";
cppcheck.check(fileSettings);
ASSERT_EQUALS("[missing.cpp:0]: (error) File not found\n", errout.str());
}
};

REGISTER_TEST(TestCppcheck)
Loading