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

Throw an error if there is an undeclared type or a template. #4

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

Flone-dnb
Copy link

@Flone-dnb Flone-dnb commented Aug 29, 2022

This PR is related to this Refureku PR.

This PR adds the function findStandardTypesErrors in FileParser. This function looks for libclang errors that occurred during translation unit parsing and that might cause incorrect type information when using reflection. For example: you are using std::vector as a field but you don't include <vector> (this will cause this field's type to be int).

With this PR when such error occurs the generator will fail and will notify the user about his error.

@Flone-dnb
Copy link
Author

I'm currently very busy at work, but I also need to test this (before considering this PR done) with reflected templated classes (when our reflected class is used as a field, for example) so that this function will not backfire on our types.

@Flone-dnb
Copy link
Author

I've decided to only check for std::vector and std::string related errors (when libclang can't find type information - usually when there is no included <vector> or <string>) because otherwise it triggers lots of errors on other types and macros after I tried it in one of my projects.

I don't have enough knowledge whether there is a better way to check all this but at least we will have protection for those types for now.

@Flone-dnb
Copy link
Author

Flone-dnb commented Aug 31, 2022

Well, I've noticed that during parsing libclang's errors might break type information (so that std::vector will be int even if we include <vector>), although seems like errors like error: unknown type name 'File_MyClass_GENERATED' don't cause any harm to this. I will see how we can ignore them (errors like error: unknown type name 'File_MyClass_GENERATED') and fail code generation on everything else.

@jsoysouvanh
Copy link
Owner

libclang throws a lot of irrelevant errors and warnings so it's very complicated to exploit it to have a consistent and coherent behaviour. That being said, I upgraded the shipped libclang library version once and as far as I remember I haven't analysed clang logs since then.

Having a generic implementation that handles all types at once sounds good to me, but I don't think handling types individually, even if they are std types is a viable solution.

As for the macro errors, they will always be fired if the generated file doesn't exist. Since the generated file doesn't exist, the macro doesn't exist neither and libclang will throw a bunch of errors like this. In the case of a regeneration, such errors should not appear.

@Flone-dnb
Copy link
Author

Having a generic implementation that handles all types at once sounds good to me, but I don't think handling types individually, even if they are std types is a viable solution.

You are right, this is why I started working on actually fixing libclang errors instead of looking for some std type errors. I will keep you updated about the process.

@Flone-dnb
Copy link
Author

Flone-dnb commented Sep 1, 2022

So, here is what I've done now:

  • We fail code generation on any libclang error.
  • When there is an error related to GENERATED macros we add those macros to the generated file (just define them) and retry libclang's parsing, after that we truncate (clear) the generated file so that it will be filled with an actual data.

With this the type information will now be processed correctly and we will no longer have ints for std::vectors and etc.
Works fine on example files, I will now try this in an actual project.

@Flone-dnb
Copy link
Author

I've added a new parameter to KodgenSettings.toml - additionalClangArguments which is just a string that will be appended to the Clang when parsing. I've added this because I needed to add -Wno-ignored-attributes for my project.

You should update library integration example after this PR will be merged: https://github.com/jsoysouvanh/RefurekuIntegration/blob/master/LibraryIntegration/RefurekuSettings.toml

@Flone-dnb Flone-dnb changed the base branch from master to dev September 2, 2022 14:23
@Flone-dnb
Copy link
Author

I've found an issue, if we are using inheritance and we haven't generated parent's data while parsing child class we will fail with an error:

expected member name or ';' after declaration specifiers

I think I know a way to fix this.

@Flone-dnb
Copy link
Author

Flone-dnb commented Sep 2, 2022

So, I've split all process into 3 steps. First, we do pre-parsing on all files (that needs to be processed), this step fills generated files with reflection macros, then only after all pre-parsing tasks were finished we do the parsing. Only after all parsing tasks were finished we start code generation, we wait for all parsing tasks because on code generation step we would need to clear generated files from empty macros that we used for proper parsing. This fixes the issue I described above.

I still need to test this with properties and test it on Linux (gcc, clang).

After I've decided to completely ignore the iterations thing
(reruning all steps multiple times) I decided to add a backwards
compatibility. I've added a new parameter in the `KodgenSettings.toml` -
`shouldFailCodeGenerationOnClangErrors` which is `false` by default.
When this setting is turned off we use the old parsing algorithm
(existed before this branch, ignores parsing errors), when it's
turned on we use the new approach where we fail on any Clang error
that occur during the parsing.
While running parsing for the tests I've noticed that there is a
situation when 1 GENERATED macro is not captured in the pre-parsing
step which fails the parsing step. I also noticed that this error
might not occur due to the fact that pre-parsing step writes
macros in parallel. Thus, I've made it so that the pre-parsing step
writes found GENERATED macros in a single-threaded manner after the
pre-parsing step but before the parsing step. Although this is indeed an
improvement, it does not solved the issue where 1 GENERATED macro was
not found. I then remembered about "iterations" thing and added a cycle.
So now, files that failed the parsing step (only failed files) will be
queued to be parsed again on the next cycle iteration. With this change,
when we rerun the pre-parsing step for failed files we have enough info
to find all GENERATED macros. Of cource I added a check where if the
number of failed files stays the same we break out of the cycle and
show an error.
When a parsing error occurres we now not only show path to the
file where error occurred but also path to the file that was parsed.
This actually helps when we are parsing file A and it includes file B
and a parsing error occurres in the file B.
@Flone-dnb
Copy link
Author

Well, I think this PR is at the finish line. I've managed to fix almost all test files (lots of them didn't included stuff that they were using) and now code generation for the new option shouldFailCodeGenerationOnClangErrors seems to be great.

There is still an issue with one test file: TemplateTemplateClassTemplate.h. There are issues with running RunRefurekuTestGenerator (code gen for test files). When using clang++ compiler in the RefurekuTestsSettings.toml lots of test files produce the following error: STL1000: Unexpected compiler version, expected Clang 13.0.0 or newer., if you update internal clang to 13.0.0 or newer this error disappears (so this is up to you whether you want to do this or not). When using msvc compiler in the RefurekuTestsSettings.toml parser fails on this file (because it includes TypeTemplateClassTemplate.h) with the following errors:

[Error] While processing the following file: Include/TemplateTemplateClassTemplate.h: 'PrivateClassTemplateInClass' is a private member of 'ClassWithNestedClassTemplate' (...\Refureku\Library\Tests\Include\TypeTemplateClassTemplate.h, line 87, column 1)
[Error] While processing the following file: Include/TemplateTemplateClassTemplate.h: 'ProtectedClassTemplateInClass' is a protected member of 'ClassWithNestedClassTemplate' (...\Refureku\Library\Tests\Include\TypeTemplateClassTemplate.h, line 87, column 1)

other test files have no errors. I honestly don't know what's causing this issue. If I remove file TemplateTemplateClassTemplate.h everything generates without errors and if I then run RefurekuTests I get this:

[  PASSED  ] 1265 tests.

So I guess everything is fine? I also tested Refureku with shouldFailCodeGenerationOnClangErrors enabled in my project and everything was great and without any errors.

In addition, I propose to set shouldFailCodeGenerationOnClangErrors to true in ReferekuTestsSettings.toml so that we will make sure we don't make any mistakes when writing these test files (but again, it's up to you to decide whether we need to do that or not).

By default shouldFailCodeGenerationOnClangErrors is set to false which means that everybody by default will be using old parser that ignores clang errors (parsing code that was before this PR). This makes this PR an opt-in feature, which I think is pretty good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants