Track Clang function types in the AST #27479

varungandhi-apple · 2019-10-02T00:36:26Z

The 3rd commit introduces a bunch of TODOs, most of which are resolved in the 4th commit.

Things that are implemented:

ClangTypeConverter is available at the AST level. It is used both when resolving types (TypeResolution) and when building types (ASTBuilder).
Including the clang types in the type equality tests.

Questions:

~~It is not clear who/what should be maintaining ownership of the ClangTypeConverter in the ASTBuilder. There are a couple of TODOs related to this. ("Is this too expensive?").~~
~~How should I further be testing this change?~~

Things that are not implemented yet but I plan to do as part of this PR (unless someone says I should make a follow-up PR about it):

Test cases -- ~~right now, the main "test" is the whole test suite passes, i.e. we don't mess up type equality anywhere.~~ added some trickier test cases
~~Setting the clang types in SILFunctionType.~~ (punted to future PR)
~~Reporting differences in the clang type in diagnostics.~~ (punted to future PR)

Since it has taken me much more time than anticipated to get to this point, I figured I'd try to get feedback sooner rather than later (even though a bunch of things are not implemented), especially given that this is a relatively large PR 😐.

varungandhi-apple · 2019-10-02T22:05:20Z

Ah, there is a compilation failure due to the removed Builtin.UnknownObject.

varungandhi-apple · 2019-10-03T01:34:35Z

Ah we are crashing when compiling SwiftPM 🙁

varungandhi-apple · 2019-10-03T02:00:18Z

We're compiling SwiftPM properly on Linux but not on OS X? 😦

include/swift/AST/ASTDemangler.h

include/swift/AST/Types.h

lib/IRGen/GenClangType.cpp

lib/SILGen/SILGenExpr.cpp

include/swift/AST/ClangTypeConverter.h

varungandhi-apple · 2019-10-17T21:21:08Z

The SwiftPM compilation crash is because of a type mismatch issue; we have a Clang type in one case and none in the other. Think I should put a more descriptive crash message there instead of just an llvm_unreachable.

varungandhi-apple · 2019-10-17T22:30:18Z

Minimal test case. Only SIG_DFL is problematic, other signals seem to work ok.

// RUN: %target-swift-frontend -typecheck %s
// REQUIRES: OS=macosx

import Darwin

func f() {
  let s: sig_t? = SIG_DFL
}

varungandhi-apple · 2019-10-18T00:02:55Z

Think I understand what the issue is. It has nothing to do with SIG_DFL and probably has everything to do with (de)serialization. We're not (de)serializing Clang types yet, so decls loaded from a different module don't have it. 🤦‍♂

The only way I see is disabling the clang function pointer check when comparing types.

However, that doesn't explain why code like let s: sig_t = SIG_IGN compiles but this doesn't. Like, how can the Optional make a difference?

Update: I talked to Robert about this and it is definitely puzzling behavior that the code with SIG_IGN compiles because the compiler shouldn't be able to magically convert one function type (which doesn't have a Clang type) to the other (which does) (or the other way around). I will file an issue for this.

varungandhi-apple · 2019-10-19T04:10:55Z

I still need to add more test cases utilizing the new flag, but the core logic should be good now.

varungandhi-apple · 2019-10-25T18:06:33Z

The LLDB failure was fixed here: swiftlang/llvm-project#24

@swift-ci please test

varungandhi-apple · 2019-11-21T04:45:19Z

@swift-ci please test Linux platform

swift-ci · 2019-11-21T05:13:34Z

Build failed
Swift Test Linux Platform
Git Sha - a89e9dac36adf14ab1945aaaff69dc97fade2899

varungandhi-apple · 2019-11-21T06:07:45Z

@swift-ci please smoke test Linux platform

varungandhi-apple · 2019-11-21T06:31:36Z

@swift-ci please test Linux platform

swift-ci · 2019-11-21T06:41:26Z

Build failed
Swift Test Linux Platform
Git Sha - a89e9dac36adf14ab1945aaaff69dc97fade2899

varungandhi-apple · 2019-11-21T06:58:00Z

The same Linux build failure seems to be happening with #28371 (build 17092) and #28374 (build 17094) as for us (builds 17093 and 17099).

varungandhi-apple · 2019-11-21T16:21:56Z

@swift-ci please clean test Linux platform

swift-ci · 2019-11-21T18:02:36Z

Build failed
Swift Test Linux Platform
Git Sha - a89e9dac36adf14ab1945aaaff69dc97fade2899

varungandhi-apple · 2019-11-21T18:27:13Z

I don't understand why we'd see such an error because of this change.

[1/11] Compiling TSCclibc libc.c
[2/12] Compiling TSCLibc libc.swift
<unknown>:0: warning: missing submodule 'TSCclibc'
/home/buildnode/jenkins/workspace/swift-PR-Linux@2/branch-master/swiftpm/Sources/TSCLibc/libc.swift:20:19: error: no such module 'TSCclibc'
@_exported import TSCclibc
                  ^

@swift-ci please test Linux platform

varungandhi-apple · 2019-11-21T20:41:56Z

Yasssss! Tests are passing. @rjmccall time for you to give this PR your blessings.

include/swift/AST/ASTDemangler.h

rjmccall · 2019-11-22T16:44:23Z

include/swift/AST/ClangSwiftTypeCorrespondence.h

@@ -0,0 +1,36 @@
+//- ClangSwiftTypeCorrespondence.cpp - Relations between Clang & Swift types -//


This isn't a .cpp file, and there's a standard form of these header comments.

there's a standard form of these header comments

I ran out of characters to add == signs around.

If you have any suggestions for rephrasing, I can do that. This is the best I could come up with to fit in 80 chars.

I think you're overthinking the filename and it could just be something like ClangTypeInfo.h - Swift-related properties of Clang types.

I think Info is very generic and doesn't hint at anything in particular.

include/swift/AST/ClangSwiftTypeCorrespondence.h

lib/AST/ASTContext.cpp

lib/AST/ClangTypeConverter.cpp

We still don't store compute or store the type anywhere; we will do so in later commits.

Note: The change in ASTBuilder::createFunctionType is functionally minor, but we need the FunctionType::Params computed _before_ the ExtInfo, so we need to shuffle a bunch of code around.

This should be a non-functional change because we shouldn't be converting protocols which aren't ObjC.

varungandhi-apple · 2019-11-22T23:38:42Z

@swift-ci please test

varungandhi-apple · 2019-12-04T01:14:06Z

@rjmccall is this good to merge?

rjmccall · 2019-12-04T02:53:40Z

See my comment in the other PR about owned, but we can do that in a follow-up; I think this is fine to merge now.

varungandhi-apple requested a review from rjmccall October 2, 2019 00:36

This comment has been minimized.

Sign in to view

varungandhi-apple closed this Oct 3, 2019

varungandhi-apple reopened this Oct 3, 2019

rjmccall reviewed Oct 3, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

rjmccall reviewed Nov 22, 2019

View reviewed changes

varungandhi-apple added 8 commits November 22, 2019 12:42

[NFC] Clean up GenClangType a little bit.

cd48a11

[NFC] Check the cache first when computing Clang types.

a4faab4

[AST] Setup AST and SIL to accomodate Clang function types.

8021795

We still don't store compute or store the type anywhere; we will do so in later commits.

[AST] Add ClangTypeConverter, computing C types for FunctionTypes.

196f358

Note: The change in ASTBuilder::createFunctionType is functionally minor, but we need the FunctionType::Params computed _before_ the ExtInfo, so we need to shuffle a bunch of code around.

[NFC] Don't compute sizes manually in GenericFunctionType::get.

7a629a8

[NFC] Add some useful crash information in CSApply.

99b7299

[NFC] Lift out common "canImportAsOptional" to a new header.

1abd35a

[NFC] Crash if we try to convert non-ObjC protocols.

ee18c00

This should be a non-functional change because we shouldn't be converting protocols which aren't ObjC.

This comment has been minimized.

Sign in to view

varungandhi-apple mentioned this pull request Nov 26, 2019

Allow spelling out a C type in the convention attribute. #28479

Merged

varungandhi-apple mentioned this pull request Dec 3, 2019

Fixes for __attribute__((ns_consumed)) block parameters #28527

Merged

varungandhi-apple merged commit e7be41d into swiftlang:master Dec 4, 2019

varungandhi-apple deleted the vg-track-clang-function-types branch December 4, 2019 16:47

varungandhi-apple mentioned this pull request Jan 8, 2021

[Sema] Diagnose type error on Clang type mismatch. #35305

Merged

		@@ -0,0 +1,36 @@
		//- ClangSwiftTypeCorrespondence.cpp - Relations between Clang & Swift types -//

Track Clang function types in the AST #27479

Track Clang function types in the AST #27479

Conversation

varungandhi-apple commented Oct 2, 2019 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented Oct 2, 2019 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented Oct 3, 2019 • edited Loading

varungandhi-apple commented Oct 3, 2019

This comment has been minimized.

varungandhi-apple commented Oct 17, 2019 • edited Loading

varungandhi-apple commented Oct 17, 2019

varungandhi-apple commented Oct 18, 2019 • edited Loading

This comment has been minimized.

varungandhi-apple commented Oct 19, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented Oct 25, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented Nov 21, 2019

swift-ci commented Nov 21, 2019

varungandhi-apple commented Nov 21, 2019

varungandhi-apple commented Nov 21, 2019

swift-ci commented Nov 21, 2019

varungandhi-apple commented Nov 21, 2019

varungandhi-apple commented Nov 21, 2019

swift-ci commented Nov 21, 2019

varungandhi-apple commented Nov 21, 2019 • edited Loading

varungandhi-apple commented Nov 21, 2019

rjmccall Nov 22, 2019

Choose a reason for hiding this comment

varungandhi-apple Nov 22, 2019

Choose a reason for hiding this comment

varungandhi-apple Nov 22, 2019

Choose a reason for hiding this comment

rjmccall Nov 22, 2019

Choose a reason for hiding this comment

varungandhi-apple Nov 22, 2019

Choose a reason for hiding this comment

This comment has been minimized.

varungandhi-apple commented Nov 22, 2019

This comment has been minimized.

This comment has been minimized.

varungandhi-apple commented Dec 4, 2019

rjmccall commented Dec 4, 2019

varungandhi-apple commented Oct 2, 2019 •

edited

Loading

varungandhi-apple commented Oct 2, 2019 •

edited

Loading

varungandhi-apple commented Oct 3, 2019 •

edited

Loading

varungandhi-apple commented Oct 17, 2019 •

edited

Loading

varungandhi-apple commented Oct 18, 2019 •

edited

Loading

varungandhi-apple commented Nov 21, 2019 •

edited

Loading