[Parse] Separate compilation condition validation and evaluation #7955

rintaro · 2017-03-07T17:28:43Z

Fixes:
https://bugs.swift.org/browse/SR-3455
https://bugs.swift.org/browse/SR-3663
https://bugs.swift.org/browse/SR-4032
https://bugs.swift.org/browse/SR-4031

Now, compilation conditions are validated at first, then evaluated. Also, in non-Swift3 mode, && now has higher precedence than ||. (i.e. A || B && C || D are evaluated as A || (B && C) || D.)

Swift3 source breaking changes:

[SR-3663] This used to be accepted and evaluate to true because of short circuit without any validation.
```
#if true || true * 12 = try Anything is OK?
print("foo")
#endif
```
In this change, remaining expressions are properly validated and diagnosed if it's invalid.

Swift3 compatibility:

[SR-4031] Compound names are diagnosed as warnings in Swift3 mode.
Previously all of these are silently accepted:

#if BAR(_:)
#elseif os(x:)(macOS)
#elseif os(Linux(foo:bar:))
#endif

Now it's diagnosed with fix-it:

warning: ignoring parentheses in compound name, which will be rejected in future version of Swift
#if BAR(_:)
       ^~~~

In Swift4 mode, it's rejected.

[SR-3663] The precedence of '||' and '&&' are still the same, and keep the short-circuit rule of Swift3. The following code evaluates to 'true'.
```
#if false || true && false
print("foo")
#endif
```

rintaro · 2017-03-07T17:29:00Z

@swift-ci Please smoke test

rintaro · 2017-03-07T18:12:52Z

@swift-ci Please smoke test

rintaro · 2017-03-07T18:20:00Z

@swift-ci Please smoke test

bitjammer · 2017-03-07T19:24:14Z

Do we have tests that Swift 3 mode is unaffected by this change? If so, LGTM.

rintaro · 2017-03-08T04:10:25Z

@bitjammer Thank you!
Since this PR contains source breaking change even in Swift3 mode,
I inserted a commit adding test cases in -swift-version 3 mode.
It passes with the current master.

You can see how this PR breaks Swift3 mode here
ce6d7f1#diff-2ff9e652924c0aaf763b9ffdf4fabd94

If these breakage are unacceptable, please let me know.

rintaro · 2017-03-08T04:11:27Z

@swift-ci Please smoke test

bitjammer · 2017-03-08T04:43:14Z

You might email swift-dev about it.

CodaFi

Well-thought-out structure is refreshing to see here. Just one little thing to discuss that isn't pressing at all.

CodaFi · 2017-03-12T01:28:05Z

lib/Parse/ParseIfConfig.cpp

My only nitpick: I think this could be made to return an Optional<StringRef> - and probably could have a more descriptive name. Right now it matches a member of ValueDecl but does something more specific than ValueDecl's implementation.

rintaro · 2017-03-14T08:11:28Z

Thanks! @CodaFi
I renamed getNameStr to getDeclRefStr, and make it to return Optional<StringRef>.

rintaro · 2017-03-14T08:11:37Z

@swift-ci Please smoke test

slavapestov · 2017-03-14T19:49:16Z

@rjmccall Please take a look

rjmccall · 2017-03-22T06:05:19Z

The Core Team believes this is a bug fix and it is acceptable to break strict source compatibility as long as the practical impact is low.

It would be nice to recognize improper precedence in Swift 3 mode and emit an a error with a fixit to add the proper parentheses.

Some people on the Core Team felt that you should add a warning about compound names. I don't think this is actually necessary.

slavapestov · 2017-03-22T06:06:43Z

Today's core team meeting must've been pretty contentious, it went way over time until 11pm PST :-)

rintaro · 2017-03-22T06:37:17Z

Thanks for discussing this!

It would be nice to recognize improper precedence in Swift 3 mode and emit an a error with a fixit to add the proper parentheses.

Sadly, parentheses can't help 😢

// test.swift
#if FOO || BAR && BAZ
  print("OK1")
#endif
#if (FOO || BAR) && BAZ
  print("OK2")
#endif
#if FOO || (BAR && BAZ)
  print("OK3")
#endif

$ swift -D BAR test.swift
OK1

We can emit a warning without fix-it, but I didn't come up with a good message for that.

rintaro · 2017-03-22T07:06:05Z

May the fix-it change the behavior in Swift3? If so, how about

warning: future version of Swift have different rule for evaluating condition; add parentheses to make the condition compatible with Swift4
#if FOO || BAR && BAZ
           ^
           (         )

https://bugs.swift.org/browse/SR-3663 https://bugs.swift.org/browse/SR-4032 https://bugs.swift.org/browse/SR-4031

Fixes: https://bugs.swift.org/browse/SR-3455 https://bugs.swift.org/browse/SR-3663 https://bugs.swift.org/browse/SR-4032 https://bugs.swift.org/browse/SR-4031 Now, compilation conditions are validated at first, then evaluated. Also, in non-Swift3 mode, '&&' now has higher precedence than '||'. 'A || B && C || D' are evaluated as 'A || (B && C) || D'. Swift3 source breaking changes: * [SR-3663] This used to be accepted and evaluate to 'true' because of short circuit without any validation. #if true || true * 12 = try Anything is OK? print("foo") #endif In this change, remaining expressions are properly validated and diagnosed if it's invalid. * [SR-4031] Compound name references are now diagnosed as errors. e.g. `#if os(foo:bar:)(macOS)` or `#if FLAG(x:y:)` Swift3 compatibility: * [SR-3663] The precedence of '||' and '&&' are still the same and the following code evaluates to 'true'. #if false || true && false print("foo") #endif

and make it to return 'Optional<StringRef>'

rintaro · 2017-03-22T17:18:45Z

For || and &&, added warning with fix-it in Swift3 mode.

@swift-ci Please smoke test

rintaro · 2017-03-22T17:30:13Z

@swift-ci Please smoke test

compilation condition in Swift3 mode.

rintaro · 2017-03-24T09:28:59Z

@swift-ci Please smoke test

rintaro · 2017-03-28T10:09:51Z

Implemented warnings for compound names in compilation condition in Swift3 mode.
Now,

#if FOO(_:)
#endif

is accepted with a warning with fix-it to remove parentheses.

test.swift:1:8: warning: ignoring parentheses in compound name, which will be rejected in future version of Swift
#if FOO(_:)
       ^~~~

rintaro · 2017-03-28T10:10:36Z

@rjmccall Any suggestion for warning messages?

rintaro · 2017-03-28T15:44:02Z

@swift-ci Please smoke test

rintaro · 2017-04-10T13:51:37Z

@rjmccall Friendly ping :)

CodaFi · 2017-04-27T02:08:58Z

@swift-ci please test source compatibility

rintaro · 2017-04-29T07:11:54Z

@swift-ci please test source compatibility

CodaFi · 2017-05-07T05:56:38Z

Once more because I doubt this patch was the cause of the breakage.

@swift-ci please test source compatibility

rintaro · 2017-05-10T15:44:17Z

Please anyone can review this?
I want to get this in Swift4 because this is source breaking change.

rintaro · 2017-05-11T18:27:29Z

@DougGregor Can I ask you to approve this, as the code owner of Parse?

jrose-apple · 2017-05-16T01:26:46Z

@bitjammer approved the early version, and @rjmccall said it's okay for Swift 4. @bitjammer, maybe you can look it over once more, and then @rintaro can land this?

bitjammer · 2017-05-16T01:27:48Z

Sure thing.

bitjammer · 2017-05-16T01:31:39Z

LGTM.

bitjammer · 2017-05-16T01:33:31Z

Thank you for this patch, @rintaro, and thanks for your patience in landing it.

rintaro · 2017-05-16T01:35:51Z

Thank you! test again
@swift-ci Please smoke test

jrose-apple requested review from CodaFi and bitjammer March 7, 2017 17:43

rintaro force-pushed the parse-ifconfig-validate branch 3 times, most recently from 81f3210 to ce6d7f1 Compare March 8, 2017 04:09

CodaFi reviewed Mar 12, 2017

View reviewed changes

rintaro force-pushed the parse-ifconfig-validate branch from ce6d7f1 to e5a6375 Compare March 14, 2017 08:06

CodaFi requested a review from rjmccall March 19, 2017 18:30

rintaro added 3 commits March 23, 2017 01:25

[test] Add several test cases for #if condition.

d23c256

https://bugs.swift.org/browse/SR-3663 https://bugs.swift.org/browse/SR-4032 https://bugs.swift.org/browse/SR-4031

[Parse] Renamed 'getNameStr' to 'getDeclRefStr'

cd0b7da

and make it to return 'Optional<StringRef>'

rintaro force-pushed the parse-ifconfig-validate branch from e5a6375 to e58c77f Compare March 22, 2017 17:16

[Parse] Add warning with fix-it for combination of '||' and '&&' in

3943b8a

compilation condition in Swift3 mode.

rintaro force-pushed the parse-ifconfig-validate branch from e58c77f to 3943b8a Compare March 24, 2017 09:28

[Parse] Add warning for compound name in compilation condition in swift3

bd3b2c6

rintaro force-pushed the parse-ifconfig-validate branch from 21dac13 to bd3b2c6 Compare March 28, 2017 15:43

rintaro mentioned this pull request May 9, 2017

[AST] Eliminate IfConfigStmt #9413

Merged

CodaFi mentioned this pull request May 15, 2017

[Parse] Allow #if to guard switch case clauses #9457

Merged

rintaro merged commit 04f31a7 into swiftlang:master May 16, 2017

rintaro deleted the parse-ifconfig-validate branch May 16, 2017 03:06

This was referenced Jun 13, 2017

[Parse] Don't invalidate compilation condition with invalid platform condition argument #10221

Merged

[4.0][Parse] Don't invalidate compilation condition with invalid platform condition argument #10356

Merged

Uh oh!

[Parse] Separate compilation condition validation and evaluation #7955

[Parse] Separate compilation condition validation and evaluation #7955

Uh oh!

Conversation

rintaro commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Mar 7, 2017

Uh oh!

rintaro commented Mar 7, 2017

Uh oh!

rintaro commented Mar 7, 2017

Uh oh!

bitjammer commented Mar 7, 2017

Uh oh!

rintaro commented Mar 8, 2017

Uh oh!

rintaro commented Mar 8, 2017

Uh oh!

bitjammer commented Mar 8, 2017

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi Mar 12, 2017

Choose a reason for hiding this comment

Uh oh!

rintaro commented Mar 14, 2017

Uh oh!

rintaro commented Mar 14, 2017

Uh oh!

slavapestov commented Mar 14, 2017

Uh oh!

rjmccall commented Mar 22, 2017

Uh oh!

slavapestov commented Mar 22, 2017

Uh oh!

rintaro commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Mar 22, 2017

Uh oh!

rintaro commented Mar 22, 2017

Uh oh!

rintaro commented Mar 24, 2017

Uh oh!

rintaro commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Mar 28, 2017

Uh oh!

rintaro commented Mar 28, 2017

Uh oh!

rintaro commented Apr 10, 2017

Uh oh!

CodaFi commented Apr 27, 2017

Uh oh!

rintaro commented Apr 29, 2017

Uh oh!

CodaFi commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented May 10, 2017

Uh oh!

rintaro commented May 11, 2017

Uh oh!

jrose-apple commented May 16, 2017

Uh oh!

bitjammer commented May 16, 2017

Uh oh!

bitjammer commented May 16, 2017

Uh oh!

bitjammer commented May 16, 2017

Uh oh!

rintaro commented May 16, 2017

Uh oh!

rintaro commented Mar 7, 2017 •

edited

Loading

rintaro commented Mar 22, 2017 •

edited

Loading

rintaro commented Mar 22, 2017 •

edited

Loading

rintaro commented Mar 28, 2017 •

edited

Loading

CodaFi commented May 7, 2017 •

edited

Loading