Skip to content

Conversation

@fliiiix
Copy link
Contributor

@fliiiix fliiiix commented Jun 23, 2025

Otherwise mypy will correctly flag code like this

def __init__(self):
  self.fooBar = None  # type: str

error: Incompatible types in assignment (expression has type "None", variable has type "str")

@github-actions github-actions bot added c++ codegen Involving generating code from schema python labels Jun 23, 2025
@fliiiix fliiiix force-pushed the bugfix/correct-str-default branch from 2622f40 to ad1b601 Compare June 25, 2025 06:15
@akb825
Copy link
Contributor

akb825 commented Jun 26, 2025

I'm not quite sure this change is correct, since there's a distinct difference between the field being unset and being explicitly set to an empty string. For example, when reading from C++ a string that was set to None would read back to nullptr, as opposed a valid string with length 0. These could have different semantic meanings depending on the intentions for the field.

Perhaps instead of changing the default value, the type should be changed to Optional[str]?

@fliiiix
Copy link
Contributor Author

fliiiix commented Jun 26, 2025

I'm not quite sure this change is correct,
I thought about this and came to the conclusion that empty string might be correcter.
Since the field is not optional. 🤷

BUT im not 100% sure, it did not have a negative impact on my code base.
Im more than happy to change it to Optional[str] which might or might not be more correct.
For my personal purposes both should work fine.

@fliiiix fliiiix force-pushed the bugfix/correct-str-default branch from ad1b601 to c6101da Compare June 26, 2025 18:06
@akb825
Copy link
Contributor

akb825 commented Jun 26, 2025

I thought about this and came to the conclusion that empty string might be correcter.
Since the field is not optional.

At least for the flatbuffer schema, strings are optional unless explicitly marked as (required). For the C++ generated code at least, a required string still creates the same code: getting the string returns a pointer rather than a reference, you're just guaranteed that a valid flatbuffer will never be set to nullptr. Also, in the case of string arrays, AFAIK the (required) isn't transitive, i.e. the required array will be guaranteed to be present to be valid, but technically the string members could be null.

FWIW I expect that arrays (represented in Python as lists) would run into the same issues. For example, with monster_test_generated.py I expect self.vectorOfLongs = None # type: List[int] should be self.vectorOfLongs = None # type: Optional[List[int]], while self.testarrayoftables = None # type: List[MonsterT] should be self.testarrayoftables = None # type: Optional[List[Optional[MonsterT]]]

fliiiix added 2 commits July 4, 2025 17:43
Otherwise mypy will correctly flag code like this

def __init__(self):
  self.fooBar = None  # type: Optional[str]

error: Incompatible types in assignment (expression has type "None", variable has type "str")
@fliiiix fliiiix force-pushed the bugfix/correct-str-default branch from c6101da to ed4b4dc Compare July 4, 2025 15:44
@fliiiix
Copy link
Contributor Author

fliiiix commented Jul 4, 2025

I updated the MR and made Lists also optional, anything else needed here before it can be merged? @akb825 @dbaileychess

@akb825
Copy link
Contributor

akb825 commented Jul 4, 2025

Looks good on my end. For an official approval, you may have more luck with @aardappel assuming that @dbaileychess is still on a hiatus.

@aardappel
Copy link
Collaborator

Yes @akb825 is correct, we should distinguish between no string and empty string. Some language APIs conflate the two for simplicity, but FlatBuffers represents them differently.

@aardappel aardappel enabled auto-merge (squash) July 4, 2025 23:46
@aardappel aardappel merged commit c15fe42 into google:master Jul 4, 2025
51 checks passed
@fliiiix fliiiix deleted the bugfix/correct-str-default branch July 7, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ codegen Involving generating code from schema python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants