Skip to content

Fix name collision with "result" in pretty printing logic in generated proto code. #10169

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

Merged

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Sep 3, 2022

When generating the C++ code from the .proto files, it was discovered that if a proto message happens to have a field named "result", "header", or "tail" then the generated C++ code would fail to compile with an error like this:

In file included from firestore.nanopb.cc:22:
pretty_printing.h: In instantiation of ‘std::string firebase::firestore::nanopb::PrintMessageField(...)’:
firestore.nanopb.cc:630:32:   required from here
pretty_printing.h:59:25: error: ‘const class std::__cxx11::basic_string<char>’ has no member named ‘ToString’
   59 |   auto contents = value.ToString(indent_level);
      |                   ~~~~~~^~~~~~~~

The root cause was that the ToString() methods defined local variables named "result", "header", and "tail" which would collide with other local variable names created for the proto messages' fields if they were also coincidentally named "result", "header", or "tail".

The fix in this PR changes those local variable names to "tostring_result", "tostring_header", and "tostring_tail", respectively, to avoid such naming conflicts in the future. Admittedly, if a future proto message happens to have a field named "tostring_result", for example, then this problem will resurface; however, that seems like an unlikely scenario and we'll deal with that if it ever happens.

When reviewing this PR, the only file that was modified by hand was Firestore/Protos/lib/pretty_printing.py; all of the other changes in this PR are the regenerated C++ code from the .proto files.

#no-changelog

@dconeybe dconeybe self-assigned this Sep 3, 2022
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.48% (96450bd) to 88.23% (5bf9be5) by -0.25%.

    15 individual files with coverage change

    FilenameBase (96450bd)Merge (5bf9be5)Diff
    common.nanopb.cc34.52%34.48%-0.04%
    document.nanopb.cc96.52%96.67%+0.14%
    firestore.nanopb.cc39.15%38.68%-0.47%
    latlng.nanopb.cc84.62%86.67%+2.05%
    leveldb_mutation_queue.cc92.42%93.56%+1.14%
    maybe_document.nanopb.cc30.95%28.89%-2.06%
    mutation.nanopb.cc77.42%75.76%-1.66%
    ordered_code.cc94.39%93.90%-0.49%
    query.nanopb.cc87.85%87.95%+0.10%
    status.nanopb.cc75.00%72.22%-2.78%
    struct.nanopb.cc7.59%7.32%-0.28%
    target.nanopb.cc45.95%45.00%-0.95%
    task.cc93.91%94.78%+0.87%
    wrappers.nanopb.cc9.26%9.40%+0.14%
    write.nanopb.cc62.11%60.23%-1.88%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/UKkUgM7JE3.html

@dconeybe dconeybe merged commit 34b729d into master Sep 6, 2022
@dconeybe dconeybe deleted the dconeybe/NanoPbPrettyPrintingResultVariableCollisionFix branch September 6, 2022 14:58
akmarinov pushed a commit to combyne/firebase-ios-sdk that referenced this pull request Sep 29, 2022
…nto inapp-customization

* 'release/9.6.0' of github.com:combyne/firebase-ios-sdk: (116 commits)
  Fix typo (firebase#10196)
  Fix zip bloat issue (firebase#10183)
  Add All and Infrastructure as product options (firebase#10185)
  [v9.6.0] Update CHANGELOGs for macOS keychain fix (firebase#10181)
  Add API tests for Analytics.sessionID() (firebase#10178)
  [CoreInternal] Add explicit generics typing for Array.Index usage (2) (firebase#10176)
  [Infra] Commits should exclude .build/ folder from nested dirs (firebase#10180)
  [Messaging] macOS keychain auth prompt fix (firebase#10166)
  Analytics 9.6.0 (firebase#10177)
  Fix index backfilling frequency (firebase#10173)
  Add protos for COUNT (firebase#10175)
  Fix name collision with "result" in pretty printing logic in generated proto code. (firebase#10169)
  firestore_client.cc: increase the kRegularBackfillDelay from 1ms to 1000ms (firebase#10170)
  [Core] Link WatchKit for watchOS (firebase#10157)
  [Core] Support watchOS lifecycle notifications (firebase#10112)
  Move zip and prerelease nightlies early (firebase#10152)
  Update versions for Release 9.6.0 (firebase#10145)
  [MLModelDownloader] Disable keychain-dependent tests on macCatalyst and macOS (firebase#10148)
  [AppCheck] Disable tests that use keychain (firebase#10146)
  Fix priority inversion issue exposed by Xcode 14 (firebase#10144)
  ...
@firebase firebase locked and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants