SE-0330: Conditionals in Collections

:upside_down_face:

I'm not sure why you're quoting that out of context John. The exact thread you cite shows a good discussion which led to the other conclusion downthread. In any case, that thread is three years old and many conversations have happened between then and now.

5 Likes

Sorry Chris, I apologise but I couldn't resist it. That comparatively short thread and in particular your later comment is indeed required reading for commenting on this review. I personally am still in favour of this pragmatic, incremental change as there is a use case for conditional test inclusion and I think a more full blown evaluation of moving Swift's preprocessor into the lexer is unlikely for the foreseeable (even though I'd fully support that and would even look at it in terms of the compiler if I thought there was backing - proceeding with this proposal would not preclude looking at the lexer later).

The patch is indeed outdated.

From the existing patch, I believed that there was sufficient signal of what the impact would be to the compiler's implementation (the basis of some of the concerns here) and the complexity of the language. Historically, there's been some amount of flexibility on the degree of completeness of implementations when evaluating proposals, depending on the complexity of the feature, etc. I made the call that the existing patch was sufficient to consider the proposal as is for review instead of leaving it in stasis or asking @johnno1962 to put in more work to update the patch when (I believed) there was sufficient signal already to evaluate the proposal. However, if there is doubt from the community that, if the patch were to be updated to top of main, that the implementation would be so different now that the proposal cannot be properly evaluated (both to its impact to the language model or compiler implementation) then that feedback is welcome in this review. That feedback can either on this thread or to direct message to me as the review manager. Based on that feedback, the course with this review can be adjusted accordingly.

2 Likes

This feels similar to the proposal to add Equatable/Hashable/Comparable conformance to tuples (in terms of letting the perfect be the enemy of the good). My review is basically the same as it was for that proposal:

  • Is the feature worth adding to the Swift language? Clearly yes.

  • Is it worth the technical debt until a more comprehensive solution comes along? IMO, that’s a question only the compiler team can really answer.

17 Likes

Thank you for bringing up that question. At first, I personally don't think this will be a technical debt, but rather a natural addition to the current model. In the initial core team's decision, Chris raised a concern:

In SE-0308, we implemented the feature without modifying AST at all. it's only Parser and Syntax change. SE-0308: Postfix '#if' expression by rintaro · Pull Request #35097 · apple/swift · GitHub Before the introduction of libSyntax, we needed to store all #if...#endif information in AST so we can do syntactic coloring etc. even in non-active #if blocks . Now that we have Syntax. Since AST doesn't need to hold syntactic information anymore, adding a use of #if ... #endif is rather simple now. Syntactic tooling like swift-format can support this feature by using the Syntax tree and it'll will probably be an additive change.

2 Likes

Right, I agree that it would be much better to do such things in Syntax instead of the AST, but this isn't what the proposal is proposing. Further, as Xiaodi points out, there are still unanswered questions with the language model, e.g. are things like this allowed?

let collection = [
#if FLAG
  "foo",
#else
  "foo": 12,
#endif
]

We can't evaluate this because there isn't a working implementation available.

-Chris

1 Like

SE-0308 was additive because it was a completely new expression node type that fit into the existing ExprSyntax hierarchy. But I don't think this change could be strictly additive for swift-format (or any SwiftSyntax user) because the elements of ArrayElementListSyntax are currently defined to only be ArrayElementSyntax. We'd have to make a breaking change somewhere in the structure, like raising the type of the elements of ArrayElementListSyntax to the type-erased Syntax wrapper so that it could contain either IfConfigDeclSyntaxes or ArrayElementSyntaxes.

This example is even scarier, because in a regular array/dictionary literal, the syntax node that encompasses the square brackets is already typed as an ArrayExprSyntax or DictionaryExprSyntax. If this example were allowed, neither of those would be satisfiable; we'd have to invent something new like an ArrayOrDictionary🤷🏻‍♂️ExprSyntax to support it.

IMO, I can't think of a non-contrived situation where being able to do the above array/dictionary dance is desirable. The goal of this proposal is to simplify conditionally including/excluding elements from a collection, not to allow a collection to conditionally be either an array or dictionary; I don't think we should allow the latter and if it were allowed I think the complexity would far outweigh the benefit. If an #if is the first thing we hit after a [, I would just determine whether the collection is an array or dictionary based on the first non-#if content that I see, and error out if any of the other blocks don't match that determination.

I'd also like to see some examples involving conditionally empty dictionaries to understand the requirements around how they're expressed, since they have the special [:] syntax. For example, would this be allowed because the content of the #if-block would provide enough context that the whole expression is a dictionary literal?

let dictionary = [
  #if FLAG
    "foo": 10,
    "bar": 20,
  #endif
]

Or would this need to be expressed like this instead, because omitting the #if-block would be equivalent to []?

let dictionary = [
  #if FLAG
    "foo": 10,
    "bar": 20,
  #else
    :
  #endif
]

And then how do you square that with more complex cases?

let dictionary = [
  #if FLAG1
    "foo": 10,
    "bar": 20,
  #else
    :
  #endif
  #if FLAG2
    :
  #else
    "meep": 30,
    "yeex": 40,
  #endif
]

I'm supportive of the ideas in this proposal in general, but like others in the thread, I think the write-up needs to be updated a good deal and the implementation needs to be refreshed before it's able to be reviewed.

8 Likes

Another semi-related question to those already raised is what the element-type joining behavior looks like for collection literals with #if conditionals inside:

class B {}
class C: B {}

// is arr of type [B], or depends on FLAG?
let arr = [
  #if FLAG
    C(),
  #else
    B(),
]

(I think this can only reasonably result in the type of arr being dependent on FLAG since expressions in the branch-not-taken needn't even compile, but it would be good to spell that out explicitly)

8 Likes

You are right the current Array/DictionaryElementSyntax cannot hold IfConfigDeclSyntax. However, we can make it additive if we introduce an Expr syntax which wraps IfConfigDeclSyntax. Since the comma in Array/DictionaryElementSyntax is optional, we could model e.g.

[
  value,
#if CONFIG
  value,
#endif
  value,
]

like

(ArrayLiteralSyntax
  (ArrayElementListSyntax
    (ArrayElementSyntax
      (IdentifierExprSyntax)
      <comma>)
    (ArrayElementSyntax
      (IfConfigCollectionElementExprSyntax
        (IfConfigDeclSyntax ... ))
      <comma=nil>))
    (ArrayElementSyntax
      (IdentifierExprSyntax)
      <comma>))))

Of course this is just an idea.

raising the type of the elements of ArrayElementListSyntax to the type-erased Syntax wrapper

may be a better option. We should discuss more about this.

A big -1 for this proposal. IMO the fact that #if in Swift is processed at syntax pass instead of lexical pass is a pain point to the compiler team in terms of maintenance, as well as a big surprise to programmers from the C language family.

If the #if should be processed at the lexical level (I think so), then patches like this proposal one after another are just hacky solutions to fix holes in the language. Not mentioning the additional test and potential compatibility cost when we finally make the shift.

If we think that #if should be processed at the syntax level (as it is today), then at least we could have a thorough discussion about all the pros and cons. This proposal is not urgent as far as I can see now.

This is a really great example. Note that (regardless of the shape of the final proposal) Swift doesn't (and can't) type check things in #if false regions. This means we can't know that the former is String to Int dictionary. The means that even the later can't work when FLAG is false, because we don't know the key/value types for the type of dictionary.

Trying to "make this work" in some cases would lead to a really fragile and unpredictable model that doesn't compose well.

One of the reasons that I'm in favor of a simple lexical #if model is that it is simple, explainable, and predictable for users. Our current model is already a pile of non-obvious special cases.

-Chris

2 Likes

Here is my take on the matter by way of "rerunning" some of the above examples using a lightweight syntax and bending some of the current rules a bit here and there:

let dictionary: [String: Int] = [
	#if flag {
		"foo": 10,
		"bar": 20
	}
]

let dictionary: [String: Int] = [] // allow this spelling when obvious to the compiler

let dictionary: [String: Int] = [
	#if flag1 {
		"foo": 10,
		"bar": 20
	}
	#if flag2 {
		// nothing here
	} else {
		"meep": 30,
		"yeex": 40
	}
]

let arr: B [
	#if flag {
		C()
	}
	B()
]

[
	value1,
	#if config {
		value2
	}
	value3
]

let dictionary: [String: Int] = [
	#if condition {
		"foo": 10,
		"bar": 20
	}
	if #available(iOS 14, *) {
		"baz": 10
	}
}

and no to:
let collection = [
#if flag {
"foo"
} else {
"foo": 12
}
]

to sum up:

  • a lightweight #if condition { ... } syntax vs more heavy current #if ... #endif syntax.
  • require explicit type annotation when it would be otherwise ambiguous or unclear - don't think it's a much of a burden.
  • relax some rules about punctuation to avoid visually disturbing commas
  • allow [ ] used instead of [:] when it is obvious to the compiler what's going on.
  • prohibit insane cases when type is changed between array and dictionary
  • and give #available the similar treatment (like in the last example)

It seems inevitable this proposal will be "returned for revision" to update the implementation and flesh out details of how various edge cases should be handled. I have tried over the last two days to build a toolchain of the original patch (as it can not easily be rebased) but this has eluded me so I can't give you anything more concrete to go on after all this time.

All of this is irrelevant however if a firm decision has already been made that the processing of preprocessor conditionals should be moved into the lexer which if this were the case, can this be communicated in the review decision and can someone be assigned to start looking at it please? In this way it can be made clear whether there is any point/need to pursue the proposal further.

1 Like

Since this would have considerable usability and compatibility impact on SwiftSyntax users like swift-format and others writing Swift source analysis tooling, I hope that such a proposed change would be given its own pitch/discussion thread and not be communicated as part of this review decision.

IMO, despite the limitations around where #if blocks can occur in the language, I think making them part of the syntax tree is still a strength of Swift's syntax model and mimicking C's preprocessor would be a mistake.

9 Likes

I hear you but this leaves us precisely where we were three years ago. Given these constraints only a tactical, incremental change such as that in this proposal (fleshed out and rebased) is a way to take a step forward at this point. This would not preclude moving to the lexer at a later date as it would be functionality the same but there also seems to be a constituency of tooling users who would not be in favour of that anyway so why hold a proposal like this in limbo in perpetuity?

Returned for Revision

This review is circling around two points:

  1. Within the current syntax-based model for condition code, the proposal cannot be reasonably evaluated because the implications are not fully clear in both the written proposal and in the implementation (which is not up-to-date on main). Because the implementation is not up-to-date, the impact of the complexity of the implementation on the compiler is difficult to assess, and the behavior of various corner cases is not clear.

  2. Whether or not the syntax-based approach for conditional code should be abandoned entirely for a different approach, such as a lexer-based one. This creates and existential question of whether this proposed change should even be considered.

The review has waded into debating the second point. To that end, @allevato raised the following reasonable concern:

The core team concurs that such a significant change in direction should have its own properly considered pitch/discussion given the potentially seismic implications. A decision along these lines will not be made as part of this review.

However, the points raised in favor of a lexer-based approach contend that the complexity of the current approach continues to stack up, and is potentially unpredictable to the user. @Chris_Lattner3 framed this perspective succinctly:

In order for this proposal to be considered, the implications of the proposed change needs to be clear. There have been several questions that have been raised in the review that would need to be answered before accepting this proposal as a refinement to the current conditional code model. Thus, the proposal is returned for revision, with the following request:

  1. Update the implementation to work with main so that the implications of the implementation can be understood.

  2. The proposal speaks to the cases brought up here, and sorts through the broader concerns brought up. An updated implementation will be helpful to this end.

With an updated proposal and implementation, the community and core team can properly weigh the cost/benefit tradeoff of this refinement to the current approach, while also providing signal for a potential discussion of moving the conditional compilation to a lexer-based approach. To be clear, it is not a forgone conclusion that a lexer-based approach is an inevitable outcome. As @allevato points out, this deserves sufficient discourse to make such a call.

On behalf of the core team, I'd like to thank both @johnno1962 as the proposal author and everyone who participated in this discussion.

12 Likes

Thank you @tkremenek for bringing this to review even if the outcome wasn't entirely conclusive. In order to avoid situations like this in future could I suggest with appropriate temerity that we introduce a "pre-review" state into the Swift Evolution pipeline. There are 34 open PRs on the swift-evolution repo that could come to review at any time. Would it be possible to schedule proposals of interest to the core team formally by the act of merging the PR (automatically notifying the proposal author and allocating the SE-NNNN number) at least two weeks ahead of time. This would create a sort of "pre-review lobby" which could be on a dashboard somewhere where authors would be motivated to fine tune their proposal, ensure the implementation is up to date and receive final comments from the community before the proposal text is finalised.

All that said, I was able to reproduce a toolchain for the original implementation last night and can give definitive if rather belated answers to some of the questions that came up during the review. I'll not post the toolchain as it is from a time before ABI stability and only builds/works with Xcode 10.1.

This was marked as an error by the implementation. Internally, parsing of collection literals read ahead an expression and a token (skipping any conditionals) to see if the token was ':' or ',' to decide the dictionary/array-ness of the literal and goes down two distinct code paths once that has been determined.

In the original implementation which you can see from my reply to @rintaro's post it was decided to code it to require the trailing ',' inside conditionals. This wasn't strictly necessary and is probably a decision I'd revisit as it creates the need to add commas inside a conditional even if it is defines the last element of the collection.

3 Likes

Hi Ted,

Could the core team provide some guidance of whether the lexer based approach should also be considered? My read of your summary here is that you're asking for a better formed version of the array-only proposal. Is the core team interested in evaluating the larger problem?

-Chris

@Ben_Cohen @Douglas_Gregor Is there any guidance available from the core team here? I'm curious which way you want to see this go.

2 Likes