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.

Sign up for GitHub

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

Jump to bottom

[Go] SbeTool generator FormatFlagsConversionMismatchException #844

Closed
neomantra opened this issue May 10, 2021 · 3 comments
Closed

[Go] SbeTool generator FormatFlagsConversionMismatchException #844

neomantra opened this issue May 10, 2021 · 3 comments

Comments

@neomantra
Copy link
Contributor

I found a fragility in the SbeTool generator for Golang. I got it to a simple reproducible case.

Process the following schema with java -Dsbe.target.language=Golang -jar sbe-all-1.22.0.jar golang_bug.xml and it will succeed... it will also succeed on all the other generators:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<messageSchema package="gobug" id="1" semanticVersion="1.0" description="Go Bug" byteOrder="littleEndian">
    <types>
        <composite name="Comp1" description="A composite">
            <type name="lmn"  primitiveType="uint16"/>
            <type name="wxyz" primitiveType="uint16"/>
        </composite>
        <composite name="messageHeader" description="Message identifiers and length of message root">z
            <type name="blockLength" primitiveType="uint16"/>
            <type name="templateId"  primitiveType="uint16"/>
            <type name="schemaId"    primitiveType="uint16"/>
            <type name="version"     primitiveType="uint16"/>
            <ref  name="o"           type="Comp1"/>
            <type name="abcd"       primitiveType="uint16"/>
            <ref  name="p"           type="Comp1"/>
        </composite>
    </types>
    <message name="barmsg" id="4">
        <field name="header"   id="1" type="messageHeader"/>
    </message>
</messageSchema>

Change <type name="abcd" to <type name="abcde" and it will fail with this exception:

Exception in thread "main" java.util.FormatFlagsConversionMismatchException: Conversion = s, Flags = 0
        at java.base/java.util.Formatter$FormatSpecifier.failMismatch(Formatter.java:4422)
        at java.base/java.util.Formatter$FormatSpecifier.checkBadFlags(Formatter.java:3151)
        at java.base/java.util.Formatter$FormatSpecifier.checkGeneral(Formatter.java:3109)
        at java.base/java.util.Formatter$FormatSpecifier.<init>(Formatter.java:2870)
        at java.base/java.util.Formatter.parse(Formatter.java:2713)
        at java.base/java.util.Formatter.format(Formatter.java:2655)
        at java.base/java.util.Formatter.format(Formatter.java:2609)
        at java.base/java.lang.String.format(String.java:3292)
        at uk.co.real_logic.sbe.generation.golang.GolangGenerator.generateTypeBodyComposite(GolangGenerator.java:2019)
        at uk.co.real_logic.sbe.generation.golang.GolangGenerator.generateComposite(GolangGenerator.java:1518)
        at uk.co.real_logic.sbe.generation.golang.GolangGenerator.generateTypeBodyComposite(GolangGenerator.java:2040)
        at uk.co.real_logic.sbe.generation.golang.GolangGenerator.generateMessageHeaderStub(GolangGenerator.java:133)
        at uk.co.real_logic.sbe.generation.golang.GolangGenerator.generate(GolangGenerator.java:163)
        at uk.co.real_logic.sbe.SbeTool.generate(SbeTool.java:315)
        at uk.co.real_logic.sbe.SbeTool.main(SbeTool.java:222)

Change <type name="abcd" to <type name="abcdef" and it will succeed!

Or keep <type name="abcd" and change to <type name="wxyz" to <type name="wxy" and it will also fail.

Relevant code link is here:
https://github.com/real-logic/simple-binary-encoding/blob/1.22.0/sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/golang/GolangGenerator.java#L2019
@neomantra
Copy link
Contributor Author

Added some instrumentation to the generator:

ENCODING  prop: 'BlockLength' longest: 11    fmt: %1s
ENCODING  prop: 'TemplateId' longest: 11    fmt: %2s
ENCODING  prop: 'SchemaId' longest: 11    fmt: %4s
ENCODING  prop: 'Version' longest: 11    fmt: %5s
ENCODING  prop: 'Lmn' longest: 3    fmt: %1s
ENCODING  prop: 'Wxy' longest: 3    fmt: %1s
ENCODING  prop: 'Abcd' longest: 3    fmt: %0s

It's the %0s format string that is causing the problem.

Looks like there's supposed to be an invariant that longest is actually the longest, but that's not happening.

Simply hard-coding the length at line 2019 works with this schema as well as my original schema.

                            .append(String.format("%4s", " "))

neomantra added a commit to neomantra/simple-binary-encoding that referenced this issue May 11, 2021
@neomantra
[Go] Refactor whitespace in GolangGenerator (real-logic#844)
6f70413 
Add functions `generateWhitespace` and `generateLongestWhitespace` to replace
repated invocations.

`generateWhitespace` ensures at least one space is created.

This change should only affect the whitespace of the output.
neomantra added a commit to neomantra/simple-binary-encoding that referenced this issue May 11, 2021
@neomantra
[Go] Refactor whitespace in GolangGenerator (real-logic#844)
fd19b3a 
Adds function `generateWhitespace` which uses the same String.format
technique, but validates the input.  It also always generates at least
one space.

This change should only affect the whitespace of the output.  It does
not attempt to change the logic that might be the underlying cause of real-logic#844.
@neomantra neomantra mentioned this issue May 11, 2021
Merged
mjpt777 pushed a commit that referenced this issue May 12, 2021
@neomantra
[Go] GolangGenerator improvements (#846)
e353366 
* [Go] Refactor whitespace in GolangGenerator (#844)

Adds function `generateWhitespace` which uses the same String.format
technique, but validates the input.  It also always generates at least
one space.

This change should only affect the whitespace of the output.  It does
not attempt to change the logic that might be the underlying cause of #844.

* [Go] Fix type name for nested composites (#847)

There was a typo in type name generation of nested
composites.  It was using the `propertyName` instead
of `propertyType`.

Add a test case and unit test. This situation wasn't
elicited before because nested property types and names
are the same in tests (e.g. `Engine`).

Prior to the change, the test case in this changeset would show:
`./MessageHeader.go:16:14: undefined: MessageHeaderC1`

Also set GO111MODULE in Makefile for Go 1.16 builds.

* [Go] fix excess import generation (#848)

The previous implementation used a single `TreeSet` member to track the import
names.  This was done as a member to not have to pass the imports to many
functions.

A given file should only have its imports.  Due to traversal of messages and
components, there are times where there are too many.  The `TreeSet` needs to
be fresh for each file.

This commit changes `imports` from a single `TreeSet` to a `Stack<TreeSet>`.

Each time where there was a line:
`imports = new TreeSet<>()`  (always near a `outputManager.createOutput` new file)

We now push that TreeSet on the stack:
`imports.push(new TreeSet<>())`

At the end of the block, that is popped.  This keeps the imports scoped to
releveant generators.

All those generators now `peek()` at the current `TreeSet`.
@neomantra neomantra mentioned this issue May 13, 2021
Closed
neomantra added a commit to neomantra/simple-binary-encoding that referenced this issue May 13, 2021
@neomantra
[Go] Fix tokens passed to generateComposite (real-logic#849)
7e46b1e 
Too many tokens were passed to `generateComposite` causing weird
errors like(real-logic#844 and real-logic#849).
@neomantra neomantra mentioned this issue May 13, 2021
Merged
mjpt777 pushed a commit that referenced this issue May 14, 2021
@neomantra
[Go] Fix tokens passed to generateComposite (#849) (#850)
1525313 
* [Go] Fix tokens passed to generateComposite (#849)

Too many tokens were passed to `generateComposite` causing weird
errors like(#844 and #849).

* [Go] Fix Group decoder style warning

The `Decode` generator for groups would produce the following code:

```
for i, _ := range o.GroupName {
```

The `gofmt` tools suggest [simplify range expression](https://github.com/golang/tools/blob/master/internal/lsp/analysis/simplifyrange/simplifyrange.go#L23):

```
A range of the form:
    for x, _ = range v {...}
will be simplified to:
    for x = range v {...}
```

This patch fixes that.
@mjpt777
Copy link
Contributor

mjpt777 commented May 28, 2021

Was this fixed by the PRs?

@neomantra
Copy link
Contributor Author

Yes, I will close these issues, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@neomantra @mjpt777