[NFC] Introduce isType and getDeclType #27195

Azoy · 2019-09-16T01:19:48Z

I've noticed that there are a lot of cases where we dance around types by grabbing their nominal type and comparing that to some stdlib decl. Let's clean this up a little bit and introduce isType so that instead of dancing around this:

if (type->getAnyNominal() == C.getIntDecl())

it becomes:

if (type->isInt())

Note: this is only applied to those stdlib types defined in KnownStdlibTypes.

I've also introduced getDeclType which is shorthand for e.g. C.getBoolDecl()->getDeclaredInterfaceType() == C.getBoolType().

@slavapestov @jrose-apple (Sorry if this is a lot to review!)

xwu · 2019-09-16T02:10:40Z

For consistency, shouldn't these be called isIntType(), etc.?

Azoy · 2019-09-16T02:50:04Z

For consistency, shouldn't these be called isIntType(), etc.?

They can be, but there was already a isBool and isVoid for precedent. It also seems kind of redundant because you're saying type twice in a sentence. "If this type is equal to Int type." It doesn't really matter to me though at the end of the day which ever way this goes.

jrose-apple

I wasn't sure at first but looking through the diff shows that (a) an awful lot of code has gotten cleaner, and (b) we were already writing helpers to do basically this anyway. I do have comments but the general approach seems reasonable.

lib/AST/ASTContext.cpp

lib/AST/Type.cpp

lib/PrintAsObjC/DeclAndTypePrinter.cpp

lib/SILGen/SILGen.cpp

lib/Sema/CSApply.cpp

lib/Sema/CSSimplify.cpp

LucianoPAlmeida · 2019-09-20T02:48:35Z

Would make sense or be useful to add something like this isStdlibType here too?

Azoy · 2019-09-21T13:29:03Z

Would make sense or be useful to add something like this isStdlibType here too?

It might make sense, maybe not in this patch though.

Azoy · 2019-09-27T00:03:08Z

Will fix conflict soon.

lib/AST/Type.cpp

lib/PrintAsObjC/DeclAndTypePrinter.cpp

lib/Sema/ConstraintSystem.cpp

jrose-apple

Last comment, I think!

lib/AST/Type.cpp

Azoy · 2019-10-08T01:03:10Z

@jrose-apple alright, should be good to go!

jrose-apple · 2019-10-08T01:30:34Z

@swift-ci Please test

jrose-apple · 2019-10-08T01:30:41Z

@swift-ci Please test source compatibility

swift-ci · 2019-10-08T01:31:13Z

Build failed
Swift Test OS X Platform
Git Sha - c2c66bf6387049607d5b32780394973afa6a6f3e

swift-ci · 2019-10-08T01:32:51Z

Build failed
Swift Test Linux Platform
Git Sha - c2c66bf6387049607d5b32780394973afa6a6f3e

Azoy · 2019-10-08T12:41:26Z

Are the failures related?

jrose-apple · 2019-10-08T15:56:26Z

Looks like it:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/stdlib/public/Darwin/Dispatch/Time.swift:19:22: error: cannot convert value of type 'mach_timebase_info_data_t' (aka 'mach_timebase_info') to expected argument type 'mach_timebase_info_t?' (aka 'Optional<UnsafeMutablePointer<mach_timebase_info>>')
mach_timebase_info(&info)
                   ^

/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift-corelibs-foundation/Foundation/NSAttributedString.swift:75:60: error: cannot convert value of type 'NSRange' (aka '_NSRange') to expected argument type 'NSRangePointer?' (aka 'Optional<UnsafeMutablePointer<_NSRange>>')
var dict = attributes(at: loc, effectiveRange: &range) as NSDictionary
                                               ^

Maybe something around the pointer conversions is being too strict?

CodaFi · 2020-04-15T00:28:23Z

We got this partially done in the linked PR. Do you have the time to rebase this and push on with the rest?

Azoy · 2020-04-15T00:37:21Z

@CodaFi I can update this weekend if that's fine.

Accidentally forgot a negation address review notes Address more of Jordan's feedback

Azoy · 2020-04-22T18:11:00Z

@CodaFi sorry this is a little later than last weekend, but I've rebased and fixed the issue upthread. Please take a look! I'll do another patch after this lands to find other places that have added more of the old way and update those.

CodaFi · 2020-04-23T18:22:59Z

lib/AST/ASTContext.cpp

+\
+Type ASTContext::get##NAME##Type() const { \
+  if (!get##NAME##Decl()) \
+    return Type(); \


This is the one thing I'm worried about. Clients of these accessors are historically terrible at handling the case where the lookup fails. That's why some of the TypeChecker entrypoints try to detect this, emit a diagnostic, and map the null type to an ErrorType. There shouldn't be any clients of this function before Sema, so maybe we should just look into doing that everywhere.

Slava pointed out in a previous commit that I did that perhaps if these accessors fail to just fail loudly because other pipelines of the compiler aren't great at handling whether or not they failed. Would that ease your concern, or should we still diagnose and provide an error type still?

CodaFi · 2020-04-23T18:23:15Z

@swift-ci test

swift-ci · 2020-04-23T19:35:51Z

Build failed
Swift Test OS X Platform
Git Sha - 690b0c0

swift-ci · 2020-04-23T19:39:21Z

Build failed
Swift Test Linux Platform
Git Sha - 690b0c0

Azoy · 2020-04-23T22:56:59Z

@CodaFi I've fixed the test issues related to function builders!

CodaFi · 2020-04-24T18:19:26Z

@swift-ci test

swift-ci · 2020-04-24T19:27:25Z

Build failed
Swift Test OS X Platform
Git Sha - 2b735c4

swift-ci · 2020-04-24T19:33:17Z

Build failed
Swift Test Linux Platform
Git Sha - 2b735c4

Azoy · 2020-04-24T19:34:09Z

Ok, I’m going to take another look at the failing tests and resolve them.

varungandhi-apple · 2020-04-27T17:06:35Z

lib/IRGen/GenClangType.cpp

@@ -303,12 +303,12 @@ ClangTypeConverter::reverseBuiltinTypeMapping(IRGenModule &IGM,
    // Handle Int and UInt specially. On Apple platforms, these correspond to
    // the NSInteger and NSUInteger typedefs, so map them back to those typedefs
    // if they're available, to ensure we get consistent ObjC @encode strings.
-    if (swiftType->getAnyNominal() == IGM.Context.getIntDecl()) {
+    if (swiftType->isInt()) {


For changes in this file, please also update the corresponding code in ClangTypeConverter.cpp, so that they don't get out of sync. 🙂 See my comment here in case you're wondering why there is this duplication.

shahmishal · 2020-10-01T06:56:08Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

jrose-apple reviewed Sep 17, 2019

View reviewed changes

Azoy force-pushed the types-need-love branch from 9c1a5cd to cb4bcd7 Compare September 21, 2019 14:57

Azoy requested a review from jrose-apple September 27, 2019 00:02

Azoy force-pushed the types-need-love branch 2 times, most recently from 4ca62f1 to 02111d3 Compare September 30, 2019 14:55

jrose-apple reviewed Sep 30, 2019

View reviewed changes

lib/AST/Type.cpp Outdated Show resolved Hide resolved

lib/PrintAsObjC/DeclAndTypePrinter.cpp Outdated Show resolved Hide resolved

lib/Sema/ConstraintSystem.cpp Show resolved Hide resolved

Azoy force-pushed the types-need-love branch 2 times, most recently from 0569522 to 1fc2b5a Compare October 6, 2019 14:36

Azoy requested a review from jrose-apple October 6, 2019 14:37

jrose-apple reviewed Oct 7, 2019

View reviewed changes

lib/AST/Type.cpp Outdated Show resolved Hide resolved

Azoy force-pushed the types-need-love branch from 1fc2b5a to c2c66bf Compare October 8, 2019 01:02

Azoy force-pushed the types-need-love branch from c2c66bf to f69d772 Compare October 8, 2019 01:04

jrose-apple self-assigned this Oct 8, 2019

jrose-apple approved these changes Oct 8, 2019

View reviewed changes

jrose-apple mentioned this pull request Oct 21, 2019

Improve warning for inferring an undesirable type #27797

Merged

jrose-apple removed their assignment Nov 1, 2019

hamishknight mentioned this pull request Nov 7, 2019

[AST] Consolidate Obj-C types on ASTContext #28128

Merged

Azoy force-pushed the types-need-love branch from f69d772 to 94b7298 Compare April 22, 2020 18:07

Introduce isType and getDeclType

690b0c0

Accidentally forgot a negation address review notes Address more of Jordan's feedback

Azoy force-pushed the types-need-love branch from 94b7298 to 690b0c0 Compare April 22, 2020 18:10

CodaFi reviewed Apr 23, 2020

View reviewed changes

BuilderTransform: Use optional declared type

2b735c4

varungandhi-apple reviewed Apr 27, 2020

View reviewed changes

Azoy changed the base branch from master to main October 1, 2020 06:59

Azoy closed this Apr 16, 2021

Azoy mentioned this pull request Apr 18, 2021

[NFC] Introduce isDecl and getDeclType #36953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] Introduce isType and getDeclType #27195

[NFC] Introduce isType and getDeclType #27195

Azoy commented Sep 16, 2019 •

edited

Loading

xwu commented Sep 16, 2019

Azoy commented Sep 16, 2019

jrose-apple left a comment

LucianoPAlmeida commented Sep 20, 2019

Azoy commented Sep 21, 2019

Azoy commented Sep 27, 2019

jrose-apple left a comment

Azoy commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

swift-ci commented Oct 8, 2019

swift-ci commented Oct 8, 2019

Azoy commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

CodaFi commented Apr 15, 2020

Azoy commented Apr 15, 2020

Azoy commented Apr 22, 2020 •

edited

Loading

CodaFi Apr 23, 2020

Azoy Apr 23, 2020

CodaFi commented Apr 23, 2020

swift-ci commented Apr 23, 2020

swift-ci commented Apr 23, 2020

Azoy commented Apr 23, 2020

CodaFi commented Apr 24, 2020

swift-ci commented Apr 24, 2020

swift-ci commented Apr 24, 2020

Azoy commented Apr 24, 2020

varungandhi-apple Apr 27, 2020

shahmishal commented Oct 1, 2020

[NFC] Introduce isType and getDeclType #27195

[NFC] Introduce isType and getDeclType #27195

Conversation

Azoy commented Sep 16, 2019 • edited Loading

xwu commented Sep 16, 2019

Azoy commented Sep 16, 2019

jrose-apple left a comment

Choose a reason for hiding this comment

LucianoPAlmeida commented Sep 20, 2019

Azoy commented Sep 21, 2019

Azoy commented Sep 27, 2019

jrose-apple left a comment

Choose a reason for hiding this comment

Azoy commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

swift-ci commented Oct 8, 2019

swift-ci commented Oct 8, 2019

Azoy commented Oct 8, 2019

jrose-apple commented Oct 8, 2019

CodaFi commented Apr 15, 2020

Azoy commented Apr 15, 2020

Azoy commented Apr 22, 2020 • edited Loading

CodaFi Apr 23, 2020

Choose a reason for hiding this comment

Azoy Apr 23, 2020

Choose a reason for hiding this comment

CodaFi commented Apr 23, 2020

swift-ci commented Apr 23, 2020

swift-ci commented Apr 23, 2020

Azoy commented Apr 23, 2020

CodaFi commented Apr 24, 2020

swift-ci commented Apr 24, 2020

swift-ci commented Apr 24, 2020

Azoy commented Apr 24, 2020

varungandhi-apple Apr 27, 2020

Choose a reason for hiding this comment

shahmishal commented Oct 1, 2020

Azoy commented Sep 16, 2019 •

edited

Loading

Azoy commented Apr 22, 2020 •

edited

Loading