fix(lang): use unicode.ToUpper in runeToUpper to handle uppercase input correctly#125
Open
karrybit wants to merge 1 commit intoprincjef:masterfrom
Open
fix(lang): use unicode.ToUpper in runeToUpper to handle uppercase input correctly#125karrybit wants to merge 1 commit intoprincjef:masterfrom
karrybit wants to merge 1 commit intoprincjef:masterfrom
Conversation
Replace the buggy runeToUpper() implementation with direct calls to
unicode.ToUpper(). The previous implementation unconditionally subtracted
32 from any rune, which corrupted uppercase characters.
This bug caused example names for generic types to be corrupted:
- ExampleID_Equal became "Example (%qual)" instead of "Example (Equal)"
- ExampleID_String became "Example (3tring)" instead of "Example (String)"
- ExampleID_Value became "Example (6alue)" instead of "Example (Value)"
The corruption occurred because runeToUpper('E') = ASCII(69) - 32 = '%'.
Changes:
- Remove buggy runeToUpper() function
- Remove unused lowerToUpper constant
- Call unicode.ToUpper() directly in splitCamel()
26da5e3 to
def1e4f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
runeToUpper()function inlang/util.gohas a bug that corrupts characters that are already uppercase.Symptoms
When generating documentation for Go code with generic types where methods are defined on inner struct types, example function names are corrupted in the output:
ExampleID_Equal()→Example (%qual)(expected:Example (Equal))ExampleID_String()→Example (3tring)(expected:Example (String))ExampleID_Value()→Example (6alue)(expected:Example (Value))Root Cause
The current implementation unconditionally subtracts the ASCII difference between lowercase and uppercase letters (32) from any rune:
When applied to an already uppercase letter, this produces incorrect results:
runeToUpper('E')= ASCII(69) - 32 = ASCII(37) ='%'runeToUpper('S')= ASCII(83) - 32 = ASCII(51) ='3'runeToUpper('V')= ASCII(86) - 32 = ASCII(54) ='6'The bug accidentally works for lowercase letters:
runeToUpper('s')= ASCII(115) - 32 = ASCII(83) ='S'✓ (correct!)Why This Bug Wasn't Caught Earlier
Most Go code follows the convention of starting method names with lowercase letters (e.g.,
structFields,myMethod). After gomarkdoc strips the type prefix, these names still start with lowercase letters, soruneToUpper()accidentally produces correct results.The bug only manifests with code patterns like:
Where example functions are named
ExampleID_Equal,ExampleID_String, etc., and after stripping the type prefixID_, we getEqual,String,Value(uppercase start).Solution
Use the standard library's
unicode.ToUpper()function, which:Testing
All existing tests pass except for two unrelated tests (
TestFunc_textScannerInitandTestFunc_ioIoutilTempFile) that appear to have pre-existing failures related to Markdown link formatting.The fix has been verified to correctly process example names for generic types with methods defined on inner structs.
Related
This fix addresses the same general category of issue as #47 and #51, which dealt with example name processing.