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

Do not use isSet in isUsed functions #655

Conversation

johannes-wolf
Copy link
Contributor

@johannes-wolf johannes-wolf commented Jul 19, 2024

Please check contribution guide first.

Description

The is*Set() functions are generated for -withWriterCode only, but are used without
further checking in is*Used() which results in non compiling code if using optional fields with -withoutWriterCode.

Always generate is*Set() for optional fields.
Alternative would be changing the expression, the C++, Java and Python generators generate for optionals <type> members to not call is*Set()
but test the optional directly.

The line triggering this bug in the Structure*.ftl files is (example from the Python generator, line 304):

def ${field.optional.isUsedIndicatorName}(self) -> bool:
  ...
  return <#if field.optional.clause??>${field.optional.clause}<#else>self.${field.optional.isSetIndicatorName}()</#if>

I've added an unused optional ... field to one of the test-structs, that triggers the bug without this change.

Type of Change

  • Bug fix
  • New feature
  • Documentation enhancement

@johannes-wolf johannes-wolf added bug Something isn't working python Python language generator java Java language generator c++ C++ language generator labels Jul 19, 2024
@johannes-wolf johannes-wolf requested a review from mikir July 19, 2024 08:38
@johannes-wolf johannes-wolf changed the title codegen: Always generate is*Set Always generate is*Set for Optional Fields Jul 19, 2024
@johannes-wolf johannes-wolf changed the title Always generate is*Set for Optional Fields Always Generate is*Set for Optional Fields Jul 19, 2024
@johannes-wolf johannes-wolf removed python Python language generator java Java language generator labels Jul 19, 2024
@johannes-wolf johannes-wolf force-pushed the bug-654-optional-without-writer-code branch 2 times, most recently from 2c51fde to 1a8288b Compare July 19, 2024 10:17
@johannes-wolf johannes-wolf added java Java language generator python Python language generator labels Jul 19, 2024
@johannes-wolf johannes-wolf force-pushed the bug-654-optional-without-writer-code branch from 1a8288b to 427d587 Compare July 19, 2024 10:56
@johannes-wolf johannes-wolf changed the title Always Generate is*Set for Optional Fields Do not use isSet in isUsed functions Jul 19, 2024
@mikir
Copy link
Contributor

mikir commented Jul 26, 2024

Thanks for that! Your fix for all generators s correct and it will be accepted without any further changes.

I just need to improve without_writer_code integration test a little bit more to check that added structure is really immutable (no non-const methods are generated).

Because I don't have access to your branch, I will create a new branch accepting there this PR and continue. OK?

@johannes-wolf
Copy link
Contributor Author

Thanks for that! Your fix for all generators s correct and it will be accepted without any further changes.

I just need to improve without_writer_code integration test a little bit more to check that added structure is really immutable (no non-const methods are generated).

Because I don't have access to your branch, I will create a new branch accepting there this PR and continue. OK?

OK 👍.

@mikir mikir changed the base branch from master to bug-654-fix-ci July 26, 2024 07:25
@mikir mikir merged commit 2864326 into ndsev:bug-654-fix-ci Jul 26, 2024
62 of 63 checks passed
@johannes-wolf johannes-wolf deleted the bug-654-optional-without-writer-code branch July 26, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working c++ C++ language generator java Java language generator python Python language generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional Fields without Writer Coder Result in Invalid Generated Code
2 participants