Renamed DictionaryLiteral to KeyValuePairs #16577

masters3d · 2018-05-12T23:41:52Z

Implementation for swiftlang/swift-evolution#850

"There is no strong motivation to deprecate. The type does not produce
active harm. Instead, it adds measurable (if small) utility and will be
part of the ABI. A sensible renaming mitigates the most problematic
issue with the type."

masters3d · 2018-05-12T23:44:45Z

@swift-ci Please smoke test
^
I don't think that worked. Can somebody kick the CI? Thanks!

slavapestov · 2018-05-13T04:19:24Z

@swift-ci Please test

slavapestov · 2018-05-13T04:19:37Z

@swift-ci Please test source compatibility

slavapestov · 2018-05-13T04:20:41Z

I agree the type itself should not be deprecated, but I think the old name should be deprecated. What do you think?

swift-ci · 2018-05-13T04:51:18Z

Build failed
Swift Test Linux Platform
Git Sha - 19eeb4a76657cf00f5a1a8dca78ea7c885ca082a

swift-ci · 2018-05-13T05:16:34Z

Build failed
Swift Test OS X Platform
Git Sha - 19eeb4a76657cf00f5a1a8dca78ea7c885ca082a

nh7a · 2018-05-13T05:27:30Z

The proposal is suggesting KeyValueList, not KeyValueArray, though.

masters3d · 2018-05-13T06:24:55Z

@nh7a I'll leave it as KeyValueArray until the final name is decided if accepted.

@slavapestov Sounds good. I misinterpreted "There is no strong motivation to deprecate".
~~1) Do you have any ideas how I can fix the typealias failing at SPM.~~
2) Do I need to worry about stdlib-stable.json?

benrimmington · 2018-05-13T11:16:35Z

include/swift/AST/Expr.h

@@ -149,7 +149,7 @@ class alignas(8) Expr {
    IsTypeDefaulted : 1,
    /// Number of comma source locations.
    NumCommas : 32 - 1 - NumExprBits,
-    /// Number of entries in the collection. If this is a DictionaryLiteral,
+    /// Number of entries in the collection. If this is a KeyValueArray,


I think this refers to DictionaryExpr (which is a subclass of CollectionExpr).

/// Number of entries in the collection. If this is a KeyValueArray, /// each entry is a Tuple with the key and value pair

The Tuple entry suggested to me that is the type and not the literal.
There are other places where the comments do refer to the same name while referring to the type.

Unless nessesary I am not going to change other usages in the project yet until CI is green.

The tuples are probably those given to any init(dictionaryLiteral:) initializer. I don't think AST/Expr.h would need to have any knowledge of the KeyValueArray type.

This is part of the confusion and reason behind the rename. I believe while AST/Expr.h has not much to do with concrete types, the comment seems to be referring to the concrete type. Since this comment is not user facing I am not too worried about it. You are welcome to change it back. I added you as a collaborator to my branch. Are you able to start the CI? Thanks!

masters3d · 2018-05-17T00:40:23Z

@swift-ci Please test source compatibility

aciidgh · 2018-05-21T21:28:23Z

@swift-ci Please test source compatibility

aciidgh · 2018-05-21T22:09:27Z

@swift-ci please test source compatibility

masters3d · 2018-05-22T16:56:31Z

none of the failures I saw are related https://ci.swift.org/job/swift-PR-source-compat-suite/1219/artifact/swift-source-compat-suite/

Aside from the Package Manager, most people do not seem to know or extend the type so I think there will not be any source compatibility issues.
https://github.com/search?p=2&q="extension+DictionaryLiteral+where"&type=Code&utf8=✓

masters3d · 2018-06-15T04:12:32Z

Updated. Not sure what to do about stdlib-stable.json

lattner · 2018-06-15T04:18:39Z

Great works @masters3d. The patch LGTM, but it would be good to get a standard library reviewer like @airspeedswift or @dabrahams to take a look.

airspeedswift

LGTM with the exception of the question of the deprecation.

airspeedswift · 2018-06-17T22:54:54Z

This all looks good to me. However, I am not sure about the deprecation in 4.2. While we could do it in 4.2, I think that's a little aggressive.

My preference would be to add the new name as a typealias in 4.2 now, and to deprecate the old name in 5.0. That way, people will be able to update their code bases in advance, and once 5.0 comes around, we can deprecate the old name in a way that means users will be able to compile without warnings with both the 4.2 and 5.0 compiler.

lattner · 2018-06-17T22:59:05Z

I don't think this type is widely used (does it appear in the source compat suite at all?) so I don't think that warning in 4.2 will be too aggressive. That said, your approach is also totally fine with me.

masters3d · 2018-06-18T04:45:33Z

Updated.

masters3d · 2018-06-27T00:00:59Z

Can this be merged?

airspeedswift · 2018-07-02T22:18:46Z

@swift-ci please test and merge

swift-ci · 2018-08-22T16:36:21Z

Build failed
Swift Test OS X Platform
Git Sha - 266de1fe81641dd2adfb4c4beb73a2cdea53b73d

benrimmington · 2018-08-22T17:30:42Z

The failure is due to a missing type change in test/api-digester/source-stability.swift.expected

 Var Dictionary.values has declared type change from LazyMapCollection<[Key : Value], Value> to Dictionary<Key, Value>.Values
+Constructor Mirror.init(_:children:displayStyle:ancestorRepresentation:) has parameter 1 type change from DictionaryLiteral<String, Any> to KeyValuePairs<String, Any>
 Constructor String.init(_:) has return type change from String? to String

airspeedswift · 2018-08-23T20:59:39Z

@akyrtzi should that api difference be firing? the one is a typealias of the other.

akyrtzi · 2018-08-23T21:11:18Z

\CC @nkcsgexi

airspeedswift · 2018-08-23T21:43:44Z

I mean, it'd be an ABI break once we stabilize the ABI, but it shouldn't be a source compatibility break.

nkcsgexi · 2018-08-24T00:01:01Z

Can we update the expected change list for now to unblock the merging of this patch?

airspeedswift · 2018-08-24T00:04:29Z

sure, just want to check if it's a bug that it flagged.

nkcsgexi · 2018-08-24T00:14:31Z

yeah, i think the api checker currently isn't smart enough to tell a type alias change is benign for source compatibility. This won't be an issue when we check abi stability because abi checker compares module dumps with all types canonicalized.

airspeedswift · 2018-08-24T01:51:37Z

Hi @masters3d can you add that entry to the expected results to fix the test for now? Thanks!

See also <swiftlang#17711>

masters3d · 2018-08-26T20:01:33Z

@airspeedswift @benrimmington updated

airspeedswift · 2018-08-26T22:03:12Z

@swift-ci please test

airspeedswift · 2018-08-26T22:03:23Z

@swift-ci please test source compatibility

swift-ci · 2018-08-26T22:13:29Z

Build failed
Swift Test OS X Platform
Git Sha - 266de1fe81641dd2adfb4c4beb73a2cdea53b73d

airspeedswift · 2018-08-26T22:33:14Z

Those failures are spurious CI artifacts, not actual failures.

airspeedswift · 2018-08-27T03:37:36Z

@swift-ci please test macOS platform

airspeedswift · 2018-08-27T14:34:46Z

@shahmishal any idea why macOS tests aren't triggering on this PR?

shahmishal · 2018-08-27T16:38:05Z

It's running - https://ci.swift.org/job/swift-PR-osx/7078/

benrimmington · 2018-08-27T17:23:15Z

@airspeedswift All checks have passed!

But I'm not sure if the source compatibility was tested.

masters3d · 2018-08-27T18:47:30Z

Thank you swift team! Thanks @benrimmington !

masters3d mentioned this pull request May 12, 2018

[Proposal] Rename DictionaryLiteral to KeyValueList swiftlang/swift-evolution#850

Merged

benrimmington reviewed May 13, 2018

View reviewed changes

masters3d force-pushed the renamed-to-KeyValueArray branch from 0b9dee7 to 1148c86 Compare May 17, 2018 00:39

masters3d mentioned this pull request May 21, 2018

add generic params to DictionaryLiteral<Key,Value> swiftlang/swift-package-manager#1579

Merged

masters3d force-pushed the renamed-to-KeyValueArray branch from 1148c86 to 10768f2 Compare June 15, 2018 04:09

masters3d changed the title ~~Renamed DictionaryLiteral to KeyValueArray~~ Renamed DictionaryLiteral to KeyValuePairs Jun 15, 2018

airspeedswift assigned airspeedswift and unassigned airspeedswift Jun 17, 2018

airspeedswift self-requested a review June 17, 2018 22:51

airspeedswift approved these changes Jun 17, 2018

View reviewed changes

airspeedswift mentioned this pull request Jul 3, 2018

[4.2][stdlib] Add KeyValuePairs typealias as prep for 5.0 rename #17711

Merged

masters3d and others added 6 commits August 24, 2018 12:15

renamed DictionaryLiteral to KeyValuePairs per SE-0214

a251c48

renamed DictionaryLiteral type tests to KeyValuePairs

2c93525

[SE-0214] Move changelog entry (Swift 4.2 => 5.0)

dda317b

[SE-0214] Update comment in AST/Expr.h

f3c3e8b

[SE-0214] Use generic typealias

99f06bd

See also <swiftlang#17711>

[SE-0214] Update source-stability.swift.expected

bbe474f

masters3d force-pushed the renamed-to-KeyValueArray branch from 266de1f to bbe474f Compare August 26, 2018 19:54

Merge branch 'master' into renamed-to-KeyValueArray

9cada0f

swiftlang deleted a comment from swift-ci Aug 26, 2018

airspeedswift merged commit a527e53 into swiftlang:master Aug 27, 2018

masters3d deleted the renamed-to-KeyValueArray branch August 27, 2018 18:47

Renamed DictionaryLiteral to KeyValuePairs #16577

Renamed DictionaryLiteral to KeyValuePairs #16577

Uh oh!

Conversation

masters3d commented May 12, 2018

Uh oh!

masters3d commented May 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented May 13, 2018

Uh oh!

slavapestov commented May 13, 2018

Uh oh!

slavapestov commented May 13, 2018

Uh oh!

swift-ci commented May 13, 2018

Uh oh!

swift-ci commented May 13, 2018

Uh oh!

nh7a commented May 13, 2018

Uh oh!

masters3d commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benrimmington May 13, 2018

Choose a reason for hiding this comment

Uh oh!

masters3d May 13, 2018

Choose a reason for hiding this comment

Uh oh!

benrimmington May 13, 2018

Choose a reason for hiding this comment

Uh oh!

masters3d May 13, 2018

Choose a reason for hiding this comment

Uh oh!

masters3d commented May 17, 2018

Uh oh!

aciidgh commented May 21, 2018

Uh oh!

aciidgh commented May 21, 2018

Uh oh!

masters3d commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masters3d commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lattner commented Jun 15, 2018

Uh oh!

airspeedswift left a comment

Choose a reason for hiding this comment

Uh oh!

airspeedswift commented Jun 17, 2018

Uh oh!

lattner commented Jun 17, 2018

Uh oh!

masters3d commented Jun 18, 2018

Uh oh!

masters3d commented Jun 27, 2018

Uh oh!

airspeedswift commented Jul 2, 2018

Uh oh!

swift-ci commented Aug 22, 2018

Uh oh!

benrimmington commented Aug 22, 2018

Uh oh!

airspeedswift commented Aug 23, 2018

Uh oh!

akyrtzi commented Aug 23, 2018

Uh oh!

airspeedswift commented Aug 23, 2018

Uh oh!

nkcsgexi commented Aug 24, 2018

Uh oh!

airspeedswift commented Aug 24, 2018

Uh oh!

nkcsgexi commented Aug 24, 2018

Uh oh!

airspeedswift commented Aug 24, 2018

masters3d commented May 12, 2018 •

edited

Loading

masters3d commented May 13, 2018 •

edited

Loading

masters3d commented May 22, 2018 •

edited

Loading

masters3d commented Jun 15, 2018 •

edited

Loading