WerWolv
fix: Broken #pragma once on import
Issue description
importing a #pragma once marked file multiple times results in multiple evaluation. (E.g. having a variable will throw an error for multiple definitions)
Issue cause
Already included files marked with #pragma once are stored in a std::set with key of custom type OnceIncludePair. The type overrides spaceship operator for map/set comparison, but uses data from api::Source pointer. The api::Source pointer is a pointer to a map element, which is stored to keep one instance for all loaded files from the same path. The issue hides in the fact that default resolver stores the instance through insert_or_assign which replaces the old data at this pointer with the new one, with a different ID. Since old comparison used IDs when added to a onceIncluded set, and the ID is now different, set order is not valid anymore, breaking set::contains and potentially other set operations.
Proposed solution
This PR compares api::Source pointers directly, instead of IDs inside of them. This ensures that no data inside will change (and therefore won't break set/map structure), while still keeping the functionality, since the pointer doesn't change.
Hello, thanks for the fix. I believe the reason we added the ID was because we deemed pointer comparison unsafe and IDs to be more unique, and therefore equivalent.
But if this proves to be equivalent and fix the proposed bug, then it should be good
Although the error is being described in detail, there is no example that exposes the error that can be tested for reproduction. Files imported that have pragma are re-imported continuously as dependencies run several levels deep. One obvious test is to import the same file twice and that didn't show any problems either. I'm not saying there is no bug, just that I can't one and nothing indicating how to trigger it is being described.
@paxcut simple repro setup:
file1.pat
#pragma once
namespace something {
auto SOME_CONST = 15;
}
Pattern editor window
import file1;
import file1;
Result
E: [ Stack Trace ]
E: /Users/zziger/Source/formats-imhex/patterns/file1.pat:4:21
E: 4 | auto SOME_CONST = 15;
E: ^
E: runtime error: Variable with name 'SOME_CONST' already exists in this scope.
E: --> in /Users/zziger/Source/formats-imhex/patterns/file1.pat:4:5
E: 4 | auto SOME_CONST = 15;
E: ^^^^
Reproduced on ImHex master branch (commit ac9f8eb) on M1 Mac
Yes, that's a situation one rarely encounters which would explain why it wasn't detected earlier. To be sure, the brief description given on the original post is not enough to describe the error. I seems it requires that the imported file is not part of the include library that is provided. For some reason those headers don't have problems with #pragma once
The bigger problem with imports and includes is the mixing of the two. I am testing a way to solve it but may take some time to implement.
I have tested this fix and it doesn't appear to fix the problem. In fact, both the code you propose and the code it substitutes behave the same way in all tests i have done. Can you supply an example where the old code fails and the new does not? The example posted above fails for both cases.
the reason the error occurs is because when it uses this code to check if header was already included, it still adds the AST nodes when there are no types defined in the children (imports of imports) of the imported file.
if (m_onceIncluded.contains( key )) {
const auto& types = m_parsedTypes[key];
if (!types.empty())
return Result::good({ {}, types });
}
The fix (and it works when tested) is to return a result even if types is empty to avoid returning ast nodes further down.
if (m_onceIncluded.contains( key )) {
const auto& types = m_parsedTypes[key];
return Result::good({ {}, types });
}
