Add Double and Int Convenience Properties #239

vermont42 · 2020-09-18T04:35:42Z

Building on the work of @ahoppen on SR-11580, this PR adds convenience properties to get Double and Int values from FloatLiteralExprSyntax and IntegerLiteralExprSyntax, respectively.

The two new properties, floatingValue and intValue, use failable initializers of Double and Int: init?<S>(_ text: S) where S : StringProtocol and init?<S>(_ text: S, radix: Int) where S : StringProtocol. These initializers fail if the Strings passed in do not represent valid Doubles or Ints. The question arises: what should floatingValue and intValue return, if anything, if initialization fails? I considered the following possibilities, ultimately choosing the fourth.

floatingValue could return Double.nan. This would make sense because, for example, "🍕.🌍" is truly not a number, floating-point or otherwise. The problem with this approach is that there is no corresponding Int.nan, which would prevent floatingValue and intValue from returning the same sort of value given invalid input. Decimal does have a nan value, so if intValue returned a Decimal rather than an Int, that would solve the problem, but I, as a consumer of the API, would find this counter-intuitive because "42" in a source file represents an Int, not a Decimal.
The return types of floatingValue and intValue could be optional. If Double or Int initialization fails, floatingValue and intValue could therefore return nil. I preferred not to impose on API consumers the burden of unwrapping.
The return types of floatingValue and intValue could be Result<Double, LiteralError> and Result<Int, LiteralError>, respectively. If Double or Int initialization fails, floatingValue and intValue could return .failure(.invalid). I preferred not to impose on API consumers the burden of switching on the returned value.
The result of the Double or Int initialization can be force-unwrapped. This causes a crash if initialization fails. Crashing is considered harmful, though not by everyone. Force-unwrapping is the most convenient option for API consumers, which is why I chose it. I observe that force-unwrapping does appear elsewhere in SwiftSyntax, for example here, here, here, and here, and I suspect that the authors of this code made similar judgments about the convenience of force-unwrapping versus the crashing risk it imposes.

Feedback on this code occurred in this PR, which I closed due to a rebase error described here.

ahoppen · 2020-09-18T08:59:48Z

swiftlang/swift#33948

@swift-ci Please test

swift-ci · 2020-09-18T10:05:12Z

Build failed
Swift Test Linux Platform
Git Sha - 7a450a8

swift-ci · 2020-09-18T10:07:59Z

Build failed
Swift Test OS X Platform
Git Sha - 7a450a8

akyrtzi · 2020-09-18T15:28:48Z

The failures are because the generated files differ:

***************
*** 1080 ****
!   public init?(_ build: (inout FloatLiteralExprSyntaxBuilder) -> Void) {
--- 1080 ----
!   public init(_ build: (inout FloatLiteralExprSyntaxBuilder) -> Void) {

Does the swift repo PR need update?

ahoppen · 2020-09-18T15:47:24Z

swiftlang/swift#33948

@swift-ci Please test

@vermont42 Could you squash the commits again?

ahoppen · 2020-09-18T15:47:57Z

swiftlang/swift#33948

@swift-ci Please test

vermont42 · 2020-09-18T16:13:58Z

@ahoppen I will do so, but I just realized that because I was using a pre-concurrency version of ExprNodes.py, my generated files were incorrect. That is why CI failed recently. I am regenerating with an updated ExprNodes.py.

ahoppen · 2020-09-18T16:34:50Z

Ah, I see. Let me know when you have regenerated the files and I will trigger CI again.

vermont42 · 2020-09-18T16:55:42Z

@akyrtzi That is my conclusion as well. I modified those Swift-repo GYB files repo more than a month ago and didn't notice until this morning that they had changed in upstream. That said, I have verified that my current Swift PR is correct in that it includes the recent concurrency changes.

ahoppen · 2020-09-18T17:21:22Z

@vermont42 What you need to do, is to rebase your swift PR so that the version in your branch contains both the concurrency changes as well as your changes and then re-generate the files for this PR. If you look at the diff for the generated files in this PR they should contain no concurrency-related changes.

vermont42 · 2020-09-18T23:02:01Z

swiftlang/swift#33948

@swift-ci Please test

I fixed the generated files to restore the recent concurrency enhancements, and tests passed locally. In the beginning of this comment, I attempt to invoke CI, not knowing whether I have permission to do so. (No prob if I don't.) When CI passes as a result of this invocation or someone else's, I will squash commits.

akyrtzi · 2020-09-18T23:06:52Z

swiftlang/swift#33948

@swift-ci Please test

rintaro

Looks good to me

xwu · 2020-09-19T02:45:07Z

Sources/SwiftSyntax/SyntaxConvenienceMethods.swift

+      $0 != "_"
+    }
+
+    return Int(textWithoutPrefixOrUnderscores, radix: radix)


I'm sorry to have missed the initial review, but this would crash for valid integer literals that are too large to be represented in Int. Is this intentional? If so, shouldn't this API be intValue and the floating-point API doubleValue and not integerValue/floatingValue (the latter implying that any valid integer or floating-point value would be returned)?

According to the Int initializer's documentation, "if the value [that the String parameter] denotes in the given radix is not representable, the result is nil." I confirmed this by running the following code:

let goodInt = Int("42", radix: 10) let badInt = Int("42424242424242424242424242424242424242424242424242424242424242424242424242", radix: 10) print("@@@ \(goodInt) @@@ \(badInt) @@@")

The output was:

@@@ Optional(42) @@@ nil @@@

Right, so force unwrapping that value will crash. Is this the intended behavior here for a valid integer literal?

A valid IntegerLiteralExprSyntax would not cause a crash.

The only way a crash would occur, via force unwrap, is if a client called integerValue on an invalid IntegerLiteralExprSyntax. However, there are guardrails in place to prevent clients from creating invalid IntegerLiteralExprSyntaxes. Those guardrails, not initially present in the PR, were the primary focus of feedback.

I might be misunderstanding then. My assumption was that 'IntegerLiteralExprSyntax' can represent any valid integer literal? If so, then calling 'integerValue' on some valid 'IntegerLiteralExprSyntax' values would crash, because not all valid integer literals can be represented as an 'Int'. For example, '42424242424242424242424242424242424242424242424242424242424242424242424242' is a valid integer literal.

IntegerLiteralExprSyntaxes represent Int literals in source files. A non-representable Int won't compile.

This isn't quite accurate; IntegerLiteralExprSyntax is not limited to what can fit into the Int type; it's any sequence of characters that is syntactically a valid integer literal. The error you're showing occurs later on at the semantic analysis phase, but the compiler and SwiftSyntax will still parse it without error. If we try the following command:

echo 'let foo = 42424242424242424242424242424242424242424242424242424242424242424242424242' | \ swiftc -frontend -emit-syntax -

we'll see this in the output:

... { "id":6, "kind":"IntegerLiteralExpr", "layout":[{ "id":5, "tokenKind": { "kind":"integer_literal", "text":"42424242424242424242424242424242424242424242424242424242424242424242424242" },

So potentialIntegerValue called on this would return nil, and integerValue would crash.

I agree with @allevato, this "overflows" error is not even coming from typechecking, it's coming from SIL code generation.
Many thanks to @xwu for catching this, I believe we need to make integerValue return an optional.

Right, integer literals are untyped; they are not “Int literals” and can now be arbitrarily large (previously, they were limited to something like 2048 bits).

The same applies for the floating-point types. They should not return rounded Double values if those values cannot represent the literal exactly.

I would recommend renaming them for clarity as well: the literals are called “integer literals” and “float literals,” so these APIs should be called “integerLiteralValue” and “floatLiteralValue.” Ideally, they would be generic over T: ExpressibleByIntegerLiteral and T: ExpressibleByFloatLiteral so users can get the value represented in any type they want.

I now agree with @xwu's concern. Accordingly, I intend to create a story in the bug tracker and raise a follow-up PR.

vermont42 · 2020-09-19T04:58:46Z

Many thanks to Xiaodi, Rintaro, Harlan, and Αργύριος for their thoughtful review and input on the PR. Extra thanks to Alex for opening the starter bug, creating the initial implementation, providing me the GYB strategy, and answering my questions.

This reverts commit 94fc5ae. This couldn't handle integer literal syntax more than Int.max e.g. 0x9e3779b97f4a0000

Revert "Add Double and Int Convenience Properties (#239)"

…ak_fix Revert "Fix for newly optional return type of `withDigits`."

Implements SR-11580

7a450a8

add generated files

fc38064

swiftlang deleted a comment from swift-ci Sep 18, 2020

fix generated files

efacdae

swiftlang deleted a comment from swift-ci Sep 18, 2020

rintaro mentioned this pull request Sep 18, 2020

Update GYB Files for SwiftSyntax Double and Int Convenience Properties swiftlang/swift#33948

Merged

rintaro approved these changes Sep 19, 2020

View reviewed changes

akyrtzi merged commit 94fc5ae into swiftlang:master Sep 19, 2020

xwu reviewed Sep 19, 2020

View reviewed changes

vermont42 mentioned this pull request Sep 22, 2020

Implement Additional Feedback on PR for SR-11580 #240

Closed

rintaro added a commit to rintaro/swift-syntax that referenced this pull request Oct 13, 2020

Revert "Add Double and Int Convenience Properties (swiftlang#239)"

bac8d8c

This reverts commit 94fc5ae. This couldn't handle integer literal syntax more than Int.max e.g. 0x9e3779b97f4a0000

rintaro mentioned this pull request Oct 13, 2020

Revert "Add Double and Int Convenience Properties (#239)" #242

Merged

rintaro added a commit that referenced this pull request Oct 13, 2020

Merge pull request #242 from rintaro/revert-pr238

d518259

Revert "Add Double and Int Convenience Properties (#239)"

swift-ci mentioned this pull request Sep 20, 2020

[SR-11580] FloatLiteralExprSyntax and IntegerLiteralExprSyntax should have computed properties that return their integer/float value #415

Closed

swift-ci mentioned this pull request May 9, 2022

[SR-13584] Make convenience methods returning an the value of an integer literal as Int and of floating literal as Float return an optional #405

Closed

adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024

Merge pull request swiftlang#239 from dylansturg/revert-237-maybe_bre…

a05fc94

…ak_fix Revert "Fix for newly optional return type of `withDigits`."

Add Double and Int Convenience Properties #239

Add Double and Int Convenience Properties #239

Uh oh!

Conversation

vermont42 commented Sep 18, 2020

Uh oh!

ahoppen commented Sep 18, 2020

Uh oh!

swift-ci commented Sep 18, 2020

Uh oh!

swift-ci commented Sep 18, 2020

Uh oh!

akyrtzi commented Sep 18, 2020

Uh oh!

ahoppen commented Sep 18, 2020

Uh oh!

ahoppen commented Sep 18, 2020

Uh oh!

vermont42 commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Sep 18, 2020

Uh oh!

vermont42 commented Sep 18, 2020

Uh oh!

ahoppen commented Sep 18, 2020

Uh oh!

vermont42 commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akyrtzi commented Sep 18, 2020

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

xwu Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

vermont42 Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

xwu Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vermont42 Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

xwu Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vermont42 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

allevato Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

akyrtzi Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

xwu Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vermont42 Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

vermont42 commented Sep 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vermont42 commented Sep 18, 2020 •

edited

Loading

vermont42 commented Sep 18, 2020 •

edited

Loading

xwu Sep 19, 2020 •

edited

Loading

xwu Sep 20, 2020 •

edited

Loading

xwu Sep 21, 2020 •

edited

Loading