Foundation: more idiomatic Swift in NSKeyedArchiver

[lhoward]

Originally when I implemented NSValue, I intended for it to be a class cluster with the concrete classes being NSSpecialValue or NSConcreteValue. At the time, Swift didn't support this pattern. Now it does support reassigning self (also see 00737ae), it makes sense to revisit this.

Also, make some other related code paths more idiomatically Swift, by eliminating force-unwrap and force-cast; at the time, I was less familiar with Swift.

Unfortunately since I cannot build the TestFoundation target on macOS, it's not possible for me to run the tests (I don't have a Linux VM handy right now).

[millenomi]

I am interested in code that aligns behavior, but I'm less interested in code that modifies e.g. object identity to look like Darwin's unless that is something that a user could detect or that changes the semantics of their code. Is it?

[millenomi]

In general, we've spent so much work moving away from the factory self pattern I'm not super interested in having people spend effort to reverse that.

[lhoward]

Fair enough. Note, though, that the _Factory extension was added by another party; this patch simply takes advantage of it. Also, having a side table is pretty inelegant and adds synchronisation overhead.

[millenomi]

Those are arguments I can buy on deviating from these principles. Let me look.

[millenomi]

@swift-ci please test

[lhoward]

Four tests failing here:

Test Case 'TestNSKeyedArchiver.test_archive_concrete_value' started at 2022-04-05 22:28:38.896
Foundation/NSKeyedUnarchiver.swift:286: Fatal error: Value was of unexpected class NSValue
Current stack trace:
0    libswiftCore.so                    0x00007f313dca1b00 _swift_stdlib_reportFatalErrorInFile + 112
1    libswiftCore.so                    0x00007f313d98c5dc <unavailable> + 1439196
2    libswiftCore.so                    0x00007f313d98c403 <unavailable> + 1438723
3    libswiftCore.so                    0x00007f313d98af10 _assertionFailure(_:_:file:line:flags:) + 411
4    libFoundation.so                   0x00007f313e5575a0 NSKeyedUnarchiver._isClassAllowed(_:allowedClasses:) + 959
5    libFoundation.so                   0x00007f313e557a50 NSKeyedUnarchiver._validateAndMapClassDictionary(_:allowedClasses:classToConstruct:) + 1331
6    libFoundation.so                   0x00007f313e5586a0 NSKeyedUnarchiver._validateAndMapClassReference(_:allowedClasses:) + 663
7    libFoundation.so                   0x00007f313e559ac0 NSKeyedUnarchiver._decodeObject(_:) + 2182
8    libFoundation.so                   0x00007f313e55bc50 NSKeyedUnarchiver._decodeObject(forKey:) + 360
9    libFoundation.so                   0x00007f313e55d0a0 NSKeyedUnarchiver._decodeObject(of:forKey:) + 324
10   libFoundation.so                   0x00007f313e55d4b0 NSKeyedUnarchiver.decodeObject(of:forKey:) + 103

[lhoward]

There was a bug in the test when constructing the list of expected classes, it was expecting the concrete type (reported by type(of:)) rather than the cluster type (which could be obtained by NSObject.classForCoder). Fixed now, although again because I cannot build or run the tests on macOS, it may require further fixes.

[lhoward]

@swift-ci please test

[AZero13]

Any updates on this @parkera

[lhoward]

Are you able to run an automated test again? Don't think I can do it

[parkera]

@swift-ci test

[lhoward]

OK, still breaking on test, investigating.

[lhoward]

OK, perhaps this will fix it...

[parkera]

@swift-ci test

[lhoward]

Seems to pass tests, I also rebased against main when fixing, ready for review I guess?

[lhoward]

Just following up on this one.

[parkera]

Thanks!