[NFC][ClangImporter] Alter importDecl API to return an optional.

[plotfi]

Inspired by #36747. I thought I'd go a little further. I have found
there is quite a lot of usage of dyn_cast_or_null in place of checking
for nullptr or using an optional. I think using an llvm::Optional is
a lot cleaner.

Work in progress (Still doesn't compile), but I wanted to post this here for now so it doesn't get lost.

@zoecarver @hyp

[CodaFi]

Not sure I agree with this choice since it turns the clang importer's result into a tri-state (what does Optional(nullptr) signal?)

Perhaps this should be swift::NullablePtr<Decl *> or some equivalent to Optional<NonNull<Decl *>> if we can make that guarantee...

[plotfi]

Not sure I agree with this choice since it turns the clang importer's result into a tri-state (what does Optional(nullptr) signal?)

Perhaps this should be swift::NullablePtr<Decl *> or some equivalent to Optional<NonNull<Decl *>> if we can make that guarantee...

I agree. Was wondering how I could avoid this "tri-state." I can try those too and push an update.

[zoecarver]

@plotfi let's think about how we use the API importDecl today. It returns a potentially null pointer. If the pointer is null, it means we failed to import the decl.

If I understand correctly, this patch gives importDecl two failure modes: if anything fails (including importing a decl) importDecl will return None, but if we have a cached failed imported, we will return {nullptr}? The distinction between someone we failed to import now and something we failed to import a while ago doesn't seem relevant to me, so I agree with Robert, this change doesn't make a lot of sense.

Further, I don't think we get a lot of benefit from having this return an optional, what's the rational there? Maybe if we were starting a new project, using optional and always having non-null pointers would make sense, but I think we're too deep into the convention of using pointers as optionals (I could certainly be swayed on this, though). Regardless: let's factor this change out into a separate commit/PR. We can land the caching change independently (I think roughly what Thomas has is good).

[plotfi]

@plotfi let's think about how we use the API importDecl today. It returns a potentially null pointer. If the pointer is null, it means we failed to import the decl.

If I understand correctly, this patch gives importDecl two failure modes: if anything fails (including importing a decl) importDecl will return None, but if we have a cached failed imported, we will return {nullptr}? The distinction between someone we failed to import now and something we failed to import a while ago doesn't seem relevant to me, so I agree with Robert, this change doesn't make a lot of sense.

Further, I don't think we get a lot of benefit from having this return an optional, what's the rational there? Maybe if we were starting a new project, using optional and always having non-null pointers would make sense, but I think we're too deep into the convention of using pointers as optionals (I could certainly be swayed on this, though). Regardless: let's factor this change out into a separate commit/PR. We can land the caching change independently (I think roughly what Thomas has is good).

Sounds good. I went a little overboard off of the caching changes and wanted to see how far up things go. Is there anything particularly you want different in the caching change or just land it? I like Robert's suggestion of using Optional<NonNull<Decl *>> or something similar.