Skip to content

Implement Additional Feedback on PR for SR-11580 #240

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Implement Additional Feedback on PR for SR-11580 #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fb68dc
implement feedback on optionality and naming
vermont42 Sep 22, 2020
4eb76f1
reflect new names and characters of literal properties
vermont42 Oct 3, 2020
b6c3592
remove must_uphold_invariant
vermont42 Oct 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Note: This is in reverse chronological order, so newer entries are added to the

## Swift 5.3

* Introduced `integerValue` and `floatingValue` properties to `IntegerLiteralExprSyntax` and `FloatLiteralExprSyntax`, respectively. Converted their `digits` and `floatingDigits` setters, respectively, into throwing functions.
* Introduced `integerLiteralValue` and `floatLiteralValue` properties to `IntegerLiteralExprSyntax` and `FloatLiteralExprSyntax`, respectively.

* Introduced `FunctionCallExprSyntax.additionalTrailingClosures` property with type `MultipleTrailingClosureElementListSyntax?` for supporting [SE-0279 Multiple Trailing Closures](https://github.com/apple/swift-evolution/blob/master/proposals/0279-multiple-trailing-closures.md).

Expand Down
4 changes: 0 additions & 4 deletions Sources/SwiftSyntax/SyntaxBuilders.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,7 @@ extension ${node.name} {
/// incrementally build the structure of the node.
/// - Returns: A `${node.name}` with all the fields populated in the builder
/// closure.
% if node.must_uphold_invariant:
public init?(_ build: (inout ${Builder}) -> Void) {
% else:
public init(_ build: (inout ${Builder}) -> Void) {
% end
var builder = ${Builder}()
build(&builder)
let data = builder.buildData()
Expand Down
26 changes: 7 additions & 19 deletions Sources/SwiftSyntax/SyntaxConvenienceMethods.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,20 @@
//===----------------------------------------------------------------------===//

public extension FloatLiteralExprSyntax {
var floatingValue: Double {
return potentialFloatingValue!
}

fileprivate var potentialFloatingValue: Double? {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One benefit of this PR is that this property is no longer needed.

var floatLiteralValue: Double? {
let floatingDigitsWithoutUnderscores = floatingDigits.text.filter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is other validation of text of which I'm unaware, it's not sufficient simply to drop all underscores. There are rules surrounding where underscores may appear, and there are combinations of underscores that are invalid. For example, 1.0e_3 is not permitted, because an underscore in the floating point exponent is not allowed. Simply dropping underscores means that you will have false positives where floatLiteralValue returns a non-nil result but in fact the literal represented is invalid.

The rules for where underscores are allowed (where, how many consecutively, etc.) aren't easily found and, even when found, may be inaccurate. For instance, according to PEP 515 (which added underscore separators to numeric literals in Python and surveyed the rules in other languages), Swift documentation says that any number of underscores are allowed between digits, but in fact they are allowed in Swift at the end of literals after the last digit as well.

This will likely require either experimentation or a careful reading of the Swift compiler's code. If you undertake this work, please do document it somewhere for posterity.

Similar issues may apply to integer literals too, although to a lesser degree because the syntax is less complex. Even if there turn out to be no such issues, it will be good to undertake the exercise of proving that and documenting it.

$0 != "_"
}
return Double(floatingDigitsWithoutUnderscores)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the String-to-Double conversion API may produce both false positives and false negatives (again, unless there is other validation of text of which I'm unaware); in other words, there will be valid literals for which this function will return nil, and invalid literals for which this function returns a value. The reason for this is that Swift's rules for numeric literals diverge from what's allowed for C's numeric literals, while conversions to and from String values use (or at least follow more closely) C's rules, and it's too late to change that behavior because it will break existing code.

As an example, .5 is not a valid Swift float literal, but ".5" is a string that can be converted to a floating-point value using Double(".5"). The reason for this particular divergence is that Swift has a leading-dot member syntax.

There are other examples listed here that will likely require some experimentation to ensure are still applicable and as described. It is likely that I have missed some of the differences too (for instance, I never did list the differences in behavior when a float literal is too large to be represented by the desired type versus a string).

Again, similar issues may apply to integer literals, and even if there turn out to be no such issues, it would be important to prove and document that.

}
}

public extension IntegerLiteralExprSyntax {
var integerValue: Int {
return potentialIntegerValue!
var isValid: Bool {
floatLiteralValue != nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest removal of isValid altogether for now; whether a float literal is a valid or invalid literal has no relationship to whether it can be converted to a value of any particular type, for reasons we discussed earlier.

It may be, however, that in your experimentation regarding the issues above, you end up having a sufficiently complete implementation validating Swift's syntactic rules surrounding literals that you can implement isValid without reference to conversion to any particular floating-point type. In that case, then by all means include an isValid member that checks for validity of the literal.

The same comment applies to the other isValid for integer literals.

}
}

fileprivate var potentialIntegerValue: Int? {
public extension IntegerLiteralExprSyntax {
var integerLiteralValue: Int? {
Copy link
Contributor

@xwu xwu Oct 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so here's my one comment on API design. I think users will want to know what value is represented by their numeric literal expression for types other than the default Int and Double. It would be very reasonable to ask what's the UInt32 value represented by a certain literal, for instance.

Therefore, we can use a (generic) method instead of a property to allow the user to pass a variable indicating the desired type:

func integerLiteralValue<T>(ofType _: T.Type) -> T
where T: ExpressibleByIntegerLiteral { ... }

Users could then write: literalExpr.integerLiteralValue(ofType: UInt32.self). Now, it's likely that users will most often want a value of the default literal types, so it's reasonable to add that default so that users don't have to type it out every time:

func integerLiteralValue<T>(ofType _: T.Type = IntegerLiteralType) -> T
where T: ExpressibleByIntegerLiteral { ... }

Now, the use site simply differs from what you have currently in this PR by a pair of parentheses. Pretty good.

However, if you were to try to implement this API, you'd find that you actually need to constrain T a bit further in order to use the String conversion APIs. It's tempting to write where T: ExpressibleByIntegerLiteral, T: LosslessStringConvertible, but in fact this raises a problem: there's no semantic guarantee that the string format from which a type can be losslessly converted is the same as the literal format (and in fact, they don't, as described above, but at least it's somewhat close for the built-in types). But in practice, we might be able to get away with it.

However, you'd run into another problem when it comes to integer literals, because if you want to pass the radix argument, you'd need to constrain to T: FixedWidthInteger instead of T: LosslessStringConvertible. This is fine, but floating-point types are expressible by integer literals (this is another way in which literals and numeric types differ from their C counterparts) but they are not fixed-width integer types; it'd be difficult to make it possible to convert from binary or octal integer literals to floating-point types without implementing your own conversions from scratch here. This is certainly not necessary for this PR and likely out of scope for this project entirely, but if we ignore floating-point types and integer literals, you can get 80% of the way just by making a few adjustments to the API design.

let text = digits.text
let (prefixLength, radix) = IntegerLiteralExprSyntax.prefixLengthAndRadix(text: text)
let digitsStartIndex = text.index(text.startIndex, offsetBy: prefixLength)
Expand Down Expand Up @@ -65,16 +61,8 @@ public extension IntegerLiteralExprSyntax {
return (decimalPrefix.count, decimalRadix)
}
}
}

public extension IntegerLiteralExprSyntax {
var isValid: Bool {
potentialIntegerValue != nil
}
}

public extension FloatLiteralExprSyntax {
var isValid: Bool {
potentialFloatingValue != nil
integerLiteralValue != nil
}
}
6 changes: 1 addition & 5 deletions Sources/SwiftSyntax/SyntaxFactory.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,7 @@ public enum SyntaxFactory {
% param_type = param_type + "?"
% child_params.append("%s: %s" % (child.swift_name, param_type))
% child_params = ', '.join(child_params)
% if node.must_uphold_invariant:
public static func make${node.syntax_kind}(${child_params}) -> ${node.name}? {
% else:
public static func make${node.syntax_kind}(${child_params}) -> ${node.name} {
% end
let layout: [RawSyntax?] = [
% for child in node.children:
% if child.is_optional:
Expand All @@ -86,7 +82,7 @@ public enum SyntaxFactory {
}
% end

% if not node.is_base() and not node.must_uphold_invariant:
% if not node.is_base():
public static func makeBlank${node.syntax_kind}() -> ${node.name} {
let data = SyntaxData.forRoot(RawSyntax.create(kind: .${node.swift_syntax_kind},
layout: [
Expand Down
42 changes: 0 additions & 42 deletions Sources/SwiftSyntax/SyntaxNodes.swift.gyb.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,29 +70,14 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
public init?(_ syntax: Syntax) {
guard syntax.raw.kind == .${node.swift_syntax_kind} else { return nil }
self._syntaxNode = syntax
% if node.must_uphold_invariant:
if !isValid {
fatalError("Instance of ${node.name} is invalid.")
}
% end
}

/// Creates a `${node.name}` node from the given `SyntaxData`. This assumes
/// that the `SyntaxData` is of the correct kind. If it is not, the behaviour
/// is undefined.
% if node.must_uphold_invariant:
/// This initializer returns nil if the invariant is not satisfied.
internal init?(_ data: SyntaxData) {
% else:
internal init(_ data: SyntaxData) {
% end
assert(data.raw.kind == .${node.swift_syntax_kind})
self._syntaxNode = Syntax(data)
% if node.must_uphold_invariant:
if !isValid {
return nil
}
% end
}

public var syntaxNodeType: SyntaxProtocol.Type {
Expand Down Expand Up @@ -122,33 +107,10 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
% end
return ${child.type_name}(childData!)
}
% if not node.must_uphold_invariant:
set(value) {
self = with${child.name}(value)
}
% end
}
% if node.must_uphold_invariant:

public enum ${child.name}Error: Error, CustomStringConvertible {
case invalid(${child.swift_name}: ${ret_type})

public var description: String {
switch self {
case .invalid(let ${child.swift_name}):
return "attempted to use setter with invalid ${child.name} \"\(${child.swift_name})\""
}
}
}

mutating public func set${child.name}(_ ${child.swift_name}: ${ret_type}) throws {
if let childSyntax = with${child.name}(${child.swift_name}) {
self = childSyntax
} else {
throw ${child.name}Error.invalid(${child.swift_name}: ${child.swift_name})
}
}
% end
%
% # ===============
% # Adding children
Expand Down Expand Up @@ -187,11 +149,7 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
/// - param newChild: The new `${child.swift_name}` to replace the node's
/// current `${child.swift_name}`, if present.
public func with${child.name}(
% if node.must_uphold_invariant:
_ newChild: ${child.type_name}?) -> ${node.name}? {
% else:
_ newChild: ${child.type_name}?) -> ${node.name} {
% end
% if child.is_optional:
let raw = newChild?.raw
% else:
Expand Down
5 changes: 0 additions & 5 deletions Sources/SwiftSyntax/SyntaxRewriter.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,7 @@ open class SyntaxRewriter {
if let newNode = visitAny(node._syntaxNode) { return newNode }
return Syntax(visit(node))
% else:
% if node.must_uphold_invariant:
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = ${node.name}(data)!
% else:
let node = ${node.name}(data)
% end
// Accessing _syntaxNode directly is faster than calling Syntax(node)
visitPre(node._syntaxNode)
defer { visitPost(node._syntaxNode) }
Expand Down
5 changes: 0 additions & 5 deletions Sources/SwiftSyntax/SyntaxVisitor.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,7 @@ open class SyntaxVisitor {
}
visitPost(node)
% else:
% if node.must_uphold_invariant:
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = ${node.name}(data)!
% else:
let node = ${node.name}(data)
% end
let needsChildren = (visit(node) == .visitChildren)
// Avoid calling into visitChildren if possible.
if needsChildren && node.raw.numberOfChildren > 0 {
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftSyntax/gyb_generated/SyntaxBuilders.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,7 @@ extension FloatLiteralExprSyntax {
/// incrementally build the structure of the node.
/// - Returns: A `FloatLiteralExprSyntax` with all the fields populated in the builder
/// closure.
public init?(_ build: (inout FloatLiteralExprSyntaxBuilder) -> Void) {
public init(_ build: (inout FloatLiteralExprSyntaxBuilder) -> Void) {
var builder = FloatLiteralExprSyntaxBuilder()
build(&builder)
let data = builder.buildData()
Expand Down Expand Up @@ -1444,7 +1444,7 @@ extension IntegerLiteralExprSyntax {
/// incrementally build the structure of the node.
/// - Returns: A `IntegerLiteralExprSyntax` with all the fields populated in the builder
/// closure.
public init?(_ build: (inout IntegerLiteralExprSyntaxBuilder) -> Void) {
public init(_ build: (inout IntegerLiteralExprSyntaxBuilder) -> Void) {
var builder = IntegerLiteralExprSyntaxBuilder()
build(&builder)
let data = builder.buildData()
Expand Down
18 changes: 16 additions & 2 deletions Sources/SwiftSyntax/gyb_generated/SyntaxFactory.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ public enum SyntaxFactory {
], length: .zero, presence: .present))
return ArrowExprSyntax(data)
}
public static func makeFloatLiteralExpr(floatingDigits: TokenSyntax) -> FloatLiteralExprSyntax? {
public static func makeFloatLiteralExpr(floatingDigits: TokenSyntax) -> FloatLiteralExprSyntax {
let layout: [RawSyntax?] = [
floatingDigits.raw,
]
Expand All @@ -629,6 +629,13 @@ public enum SyntaxFactory {
return FloatLiteralExprSyntax(data)
}

public static func makeBlankFloatLiteralExpr() -> FloatLiteralExprSyntax {
let data = SyntaxData.forRoot(RawSyntax.create(kind: .floatLiteralExpr,
layout: [
RawSyntax.missingToken(TokenKind.floatingLiteral("")),
], length: .zero, presence: .present))
return FloatLiteralExprSyntax(data)
}
public static func makeTupleExpr(leftParen: TokenSyntax, elementList: TupleExprElementListSyntax, rightParen: TokenSyntax) -> TupleExprSyntax {
let layout: [RawSyntax?] = [
leftParen.raw,
Expand Down Expand Up @@ -757,7 +764,7 @@ public enum SyntaxFactory {
], length: .zero, presence: .present))
return DictionaryElementSyntax(data)
}
public static func makeIntegerLiteralExpr(digits: TokenSyntax) -> IntegerLiteralExprSyntax? {
public static func makeIntegerLiteralExpr(digits: TokenSyntax) -> IntegerLiteralExprSyntax {
let layout: [RawSyntax?] = [
digits.raw,
]
Expand All @@ -767,6 +774,13 @@ public enum SyntaxFactory {
return IntegerLiteralExprSyntax(data)
}

public static func makeBlankIntegerLiteralExpr() -> IntegerLiteralExprSyntax {
let data = SyntaxData.forRoot(RawSyntax.create(kind: .integerLiteralExpr,
layout: [
RawSyntax.missingToken(TokenKind.integerLiteral("")),
], length: .zero, presence: .present))
return IntegerLiteralExprSyntax(data)
}
public static func makeBooleanLiteralExpr(booleanLiteral: TokenSyntax) -> BooleanLiteralExprSyntax {
let layout: [RawSyntax?] = [
booleanLiteral.raw,
Expand Down
6 changes: 2 additions & 4 deletions Sources/SwiftSyntax/gyb_generated/SyntaxRewriter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2128,8 +2128,7 @@ open class SyntaxRewriter {

/// Implementation detail of visit(_:). Do not call directly.
private func visitImplFloatLiteralExprSyntax(_ data: SyntaxData) -> Syntax {
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = FloatLiteralExprSyntax(data)!
let node = FloatLiteralExprSyntax(data)
// Accessing _syntaxNode directly is faster than calling Syntax(node)
visitPre(node._syntaxNode)
defer { visitPost(node._syntaxNode) }
Expand Down Expand Up @@ -2199,8 +2198,7 @@ open class SyntaxRewriter {

/// Implementation detail of visit(_:). Do not call directly.
private func visitImplIntegerLiteralExprSyntax(_ data: SyntaxData) -> Syntax {
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = IntegerLiteralExprSyntax(data)!
let node = IntegerLiteralExprSyntax(data)
// Accessing _syntaxNode directly is faster than calling Syntax(node)
visitPre(node._syntaxNode)
defer { visitPost(node._syntaxNode) }
Expand Down
6 changes: 2 additions & 4 deletions Sources/SwiftSyntax/gyb_generated/SyntaxVisitor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2868,8 +2868,7 @@ open class SyntaxVisitor {

/// Implementation detail of doVisit(_:_:). Do not call directly.
private func visitImplFloatLiteralExprSyntax(_ data: SyntaxData) {
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = FloatLiteralExprSyntax(data)!
let node = FloatLiteralExprSyntax(data)
let needsChildren = (visit(node) == .visitChildren)
// Avoid calling into visitChildren if possible.
if needsChildren && node.raw.numberOfChildren > 0 {
Expand Down Expand Up @@ -2946,8 +2945,7 @@ open class SyntaxVisitor {

/// Implementation detail of doVisit(_:_:). Do not call directly.
private func visitImplIntegerLiteralExprSyntax(_ data: SyntaxData) {
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil.
let node = IntegerLiteralExprSyntax(data)!
let node = IntegerLiteralExprSyntax(data)
let needsChildren = (visit(node) == .visitChildren)
// Avoid calling into visitChildren if possible.
if needsChildren && node.raw.numberOfChildren > 0 {
Expand Down
Loading