Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

[Chore] Fork update #4

Closed
wants to merge 72 commits into from
Closed

[Chore] Fork update #4

wants to merge 72 commits into from

Conversation

johnxnguyen
Copy link

This PR contains all new upstream changes from the source repository.

Do NOT squash this merge.

thomasvl and others added 30 commits May 3, 2019 09:55
This still allows it for the things that can custom support decoding from
null, but the test isn't completely clear on what should happen for those.

Fixes apple#869
To ensure compatibility with Swift 3 and Swift 4, the
Makefile has had both of the following for a while:
  "swift build --clean" was used in Swift 3
  "swift package clean" replaced it in Swift 4

Of course, this also meant the Makefile had to ignore errors from
both commands, since either might fail.

Now that we're no longer supporting Swift 3, we can
remove the Swift 3 command and respect errors from
the Swift 4 (and later) command.
Fixes a warning from the Swift 5 compiler about
this "var" that is never mutated.  (The fix is correct
for Swift 4 as well, the only change in Swift 5 is the new
warning.)
Previously, this code directly invoked C standard
library `sprintf` to convert floating-point numbers to text
formats.  The new code uses Swift standard library routines
that are both faster and more accurate (in Swift 4.2 and later).
In particular, most of `DoubleFormatter` has been removed
and the remainder renamed to to `DoubleParser`.

Swift 4.2 and later:

New versions of Swift have floating-point formatting
logic that is both fast and always generates "optimal"
output that is guaranteed to parse back to exactly the
same value.  Note that `description` and `debugDescription`
generate the exact same output for all finite values.

In particular, the following is now a fast way to
append the UTF-8 form of a floating point number to
a Data object:
```
  data.append(contentsOf: value.debugDescription.utf8)
```

Swift 4.1 and earlier:

Correctness:  The snippet above uses `debugDescription`
since Swift 4.1 and earlier did not always produce enough
digits for `description` to ensure round-trip accuracy.

Performance:  The new code is a modest pessimization in Swift 4.1
and earlier; the C library was used previously because Swift's
standard library routines for floating-point formatting were not
very fast.
…rmatting

The new float formatting code writes "1.0" instead of "1", for example.
This is not a significant difference, so I've updated the tests to match.
The previous code populated float/double fields with
integer values that could always exploit fast paths in
the JSON/Text formatting logic.

This version generates values that look like
```
   200.200, 201.201, 202.202, ...
```
which are actual floating point numbers, although with less
variation than I would like.

Note:

You could make this even more stressful by using logic like
```
  // E.g., 1.23456789123456789e-30, ... , 1.23456789123456789e+29
  message.field$num = 1.23456789123456789e$(( (num % 60) - 30 ))
```

Such numbers would exercise the slowest paths in both
the C++ and Swift implementations:

C++: Google's C++ code formats float/double with 6 and 15 digits
respectively, then parses the result back and reformats with 9 or 17
digits if the result does not round-trip.  Numbers that require
lots of digits thus end up performing two formats and a parse
for every field written.

Swift: Swift's low-level float/double formatting is fast and always
generates the correct number of digits.  However, the value gets
copied into a Swift String object which can only avoid memory allocation
if the result is short enough.  Values with lots of digits pay about an
extra 50% for memory allocation overhead.
Swift 5: String.characters.count => String.count
Podspec was updated to use the newer support to list multiple supported
versions instead.
Based on protocolbuffers/protobuf a03d332aca5d33c5d4b2cd25037c9e37d57eff02
…ary.

The runtime doesn't actually use these types/enums, but this should make things
easier for developers that try to generate from .proto files that include
options to annotations of files, fields, messages, etc. as they will no longer
have to generate the descriptor.pb.swift themselves and can just use the one
provided by the runtime.

This does *not* capture the binary descriptor info into the generated code
like some other languages do for protobuf, this just includes the types
defined in descriptor.proto.

Fixes apple#727
If the generated files ever change, it could break the tests, instead
use the proto crafted for the tests directly.
Using 1be79eefd5d94db7bada646a293b3fa42546763e.

The conformance tests use the new fields, so things appear to fail with the
update.
In the header of all generated files the message pointing to the
SwiftProtobuf repo has a typo: "documenation" should be "documentation".

This change fixes the typo in FileGenerator.swift and updates the
generated and reference protobufs.
main.swift has a check also, so there is no need to maintain this within the
Makefile any more (and it needed updating to handle version 3.10 anyways).
This is a workaround for apple#904, the underlying problem is still there, but this
makes the generated source file for testing "work" in that it doesn't hit the
compile failure. A real solution is still needed for large enums.
Remove runtime code for Swift 4.0.
Update the OneofGenerator to skip generating 4.0 specific code.
Regenerate regenerate the .pb.swift files to remove the 4.0 support.
This reverts commit 687cff5.

SwiftPM only has constants for 4.0 and 4.2 (as well as the compiler only taking
those values (and not 4.1)). So to drop the 4.0 from there, it would actually
mean moving up to 4.2 which is more agressive than our policy, so it seem like
we have to keep 4.0 support because you can't tell the SwiftPM or the compiler
to be atleast 4.1. So we can't drop support until we can move all the way up
to 4.2.
thomasvl and others added 26 commits November 22, 2019 13:48
JSON/TextFormat performance improvements when encoding messages with submessage fields.  Biggest wins (up to 5x) are for repeated small submessages.
When compiling with swift version below 5.0, it complains that `Data.withUnsafeBytes` is internal and cannot be referenced from an `@inlinable` function, from https://github.com/apple/swift-protobuf/blob/master/Sources/SwiftProtobuf/Message%2BBinaryAdditions.swift#L146, which is newly introduced by apple@7f8daf3

In fact, `Data.withUnsafeBytes` is defined inside `Data+Extensions` (https://github.com/apple/swift-protobuf/blob/master/Sources/SwiftProtobuf/Data%2BExtensions.swift#L19) as internal, but not `@usableFromInline`. And according to  Apple's documentation, `@inlinable` code "can interact with internal symbols declared in the same module that are marked with the usableFromInline attribute" (https://docs.swift.org/swift-book/ReferenceManual/Attributes.html).

Compiling with XCode 10.2 (using swift 5.0) and above does not reproduce this issue while compiling with XCode 10.1 (using swift 4.2) does.

References
XCode version and Swift version mapping: https://developer.apple.com/xcode/whats-new/#:~:targetText=Xcode%2010.1%20includes%20Swift%204.2.
xcode-version and swift-version flags: http://go/swift-monthly#swift-version-attribute-has-been-removed
When generating TextFormat for unknown fields, the attempt to see if a length
delimited field is a submessage was building up the unknown data pointlessly,
instead discarded the data to skip this overhead.
Use raw pointers instead of typed pointers.
The TextFormat encoding attempts to detect when a length delimited field appears
to be a message and then decode it as such. This means a well crafted message
could be fed to a process in binary format to attack it if one knows in some
condition it will generate the TextFormat (say for logging). This avoids the
problem by limiting the recursion when doing this additional decoding.
Also add an extension on Data to provide some helpers for writing tests.
Used commit 04a11fc91668884d1793bff2a0f72ee6ce4f5edd
https://developer.apple.com/documentation/swift/dictionary/2894528-subscript
says it goes back to Xcode 9, so at this point, our min Swift version should be
ensuring things always support it.

The only other place that it might make sense to use this appears to be within
the SimpleExtensionMap, but the values looked up get filtered before adding to
them, so it seems like the fast path might for not being in the map might still
be worth while.
Normally, `jsonString()` delegates to `jsonUTF8Data()`, which relies
on our efficient general JSON encoder.  But when the message type
has a custom JSON encoding, that encoding is produced directly as a String,
so the delegation unnecessarily translated string->data->string.

While here, make `init(jsonUTF8Data:options:)` a bit easier to read.
This will help ensure that both work exactly the same.  There
have been a couple of minor bugs recently where preconditions
were being checked differently in the two paths.

While here, I noticed that we were not correctly verifying that
decoded JSON was correctly re-encoded.

Fixing that in turn revealed a test bug:  I was asserting that
signed zeros were always preserved, but they are currently not.
By my reading of the spec, because -0.0 == +0.0, signed zeros
should be treated as default zero values and be omitted.
JSON improvements:  test both string- and data-based APIs, fix inconsistencies
@johnxnguyen
Copy link
Author

Closing due to fork syncing via merging upstream/master

@johnxnguyen johnxnguyen closed this Jan 9, 2020
@johnxnguyen johnxnguyen deleted the fork-update branch January 9, 2020 07:29
@CLAassistant
Copy link

CLAassistant commented Apr 13, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 9 committers have signed the CLA.

✅ johnxnguyen
❌ ydnar
❌ glbrntt
❌ tbkka
❌ dnkoutso
❌ thomasvl
❌ Lukasa
❌ ziwei2041
❌ FlamingHairball
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants