Advice on how to deal with a null dereference in the constraint system

Development Compiler
,
1

Hello!

I've made some progress chasing down a compiler crash I've been experiencing. However, I think I've reached the point where I need some advice to understand what the best way forward is.

The problem is in the constraint system. There are a number of places where I believe the result of this function is unconditionally dereferenced.

std::optional<FunctionArgApplyInfo> Solution::getFunctionArgApplyInfo(ConstraintLocator *locator) const

My particular crash is coming from this one:

ArgumentMismatchFailure(const Solution &solution, Type argType,
                          Type paramType, ConstraintLocator *locator,
                          FixBehavior fixBehavior =
                              FixBehavior::Error)
      : ContextualFailure(solution, argType, paramType, locator, fixBehavior),
        Info(*getFunctionArgApplyInfo(getLocator())) {}

I briefly investigated modifing ArgumentMismatchFailure from FunctionArgApplyInfo to std::optional<FunctionArgApplyInfo>. It will involve changes in quite a few places. Worse, because this is an internal detail of the constraint system, it is hard for me to expose this particular detail via the unit test system.

Does anyone have any ideas on what I should do next?

1 Like
2

Is getLocator defined in the parent class ContextualFailure?

If so, as It is being called during initialization, I would make sure that what it returns is not null.

1 Like
3

This is a good suggestion. And, from it, I have found numerous other code paths that will crash if getLocator() returns null. However, this is not the case in my particular situation.

Isn't the problem higher-level though? There are actually many code paths within getFunctionArgApplyInfo that can return null, and all will crash.

1 Like
4

I believe @hamishknight made this an unconditional deference at some point because ArgumentMismatchFailure is supposed to be anchored on a locator that ends on ArgToParam which should always work for getFunctionArgApplyInfo if we have a different locator for this particular fix, that's the underlying issue here.

An easy way to check that would be to use -Xfrontend -debug-constraint and see Fixes: section of the solution, it would print each fix and its locator.

3 Likes
5

Ah ha! I tried this and it produced an enormous amount of output. So, I'm truncating to the last bit right before the crash. Interestingly, this now also prints an assertion failure!

Overload choices:
  locator@0x1569ea850 [DeclRef@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:44] with EmpireTests.(file).TestRecord extension.select(in:a:).a@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:29:63 as a: String
  locator@0x1569ea200 [DeclRef@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:10] with EmpireTests.(file).TestRecord extension.select(in:a:).context@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:29:34 as context: TransactionContext
  locator@0x1569eaa10 [Call@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32 → apply function → constructor member] with Empire.(file).Query.init(_:last:)@/Users/matt/Developer/Empire/Sources/Empire/Query.swift:42:9 as Query<Pack{/* shape: $T2 */ repeat $T2}, $T3>.Type.init: ($T9, ComparisonOperator<$T8>) -> Query<Pack{/* shape: $T2 */ repeat $T2}, $T3>
  locator@0x1569ea278 [UnresolvedDot@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:18 → member] with Empire.(file).TransactionContext.select(query:)@/Users/matt/Developer/Empire/Sources/Empire/Store.swift:74:14 as TransactionContext.select: (Query<Pack{$T14}, $T13>) throws -> sending [$T11]
Constraint restrictions:
  Query<<<hole>>> to Query<<<hole>>> is [deep equality]
  Array<TestRecord> to Array<TestRecord> is [deep equality]
Trailing closure matching:
  locator@0x1569ea250 [UnresolvedDot@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:18 → apply argument]: forward
  locator@0x1569ea978 [Call@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32 → apply argument]: forward
  locator@0x156c49df8 [Call@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:18 → apply argument]: forward
Opened types:
  locator@0x1569ea418 [Type@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32] opens 'Last' (τ_0_1) -> <<hole>> [from $T3], 'each Component' (each τ_0_0) -> Pack{} [from $T2]
  locator@0x1569eaa10 [Call@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32 → apply function → constructor member] opens 'Last' (τ_0_1) -> <<hole>> [from $T8], 'each Component' (each τ_0_0) -> Pack{} [from $T7]
  locator@0x1569ea278 [UnresolvedDot@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:18 → member] opens 'Last' (τ_0_2) -> <<hole>> [from $T13], 'Record' (τ_0_0) -> TestRecord [from $T11], 'each Component' (each τ_0_1) -> Pack{} [from $T12]
Defaulted constraints: locator@0x1569ea490 [Type@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32 → generic parameter 'Last']
Fixes:
  [fix: allow argument to parameter type conversion mismatch] @ locator@0x156c493d8 [Call@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32 → apply argument → comparing call argument #0 to parameter #1]
  [fix: default generic argument 'Last' to 'Any'] @ locator@0x1569ea418 [Type@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:30:32]
Assertion failed: (!HasOriginalArgs && "Query original args instead"), function isTrailingClosureIndex, file ArgumentList.h, line 434.

I've added this to the issue too.

So, it sounds like what I should be looking for isn't to fix the null-derefs, but to address the reason why they are happening in the first place?

6

Yes the intention here is that an AllowArgumentMismatch fix should only ever be recorded for a mismatch in a call, in which case we ought to always be able to retrieve information about that call through getFunctionArgApplyInfo.

The assertion:

Assertion failed: (!HasOriginalArgs && "Query original args instead"), function isTrailingClosureIndex, file ArgumentList.h, line 434.

Indicates that we're trying to re-typecheck an already typechecked AST (an un-typechecked argument list should never have original arguments set), so that's likely where the issue is.

2 Likes
7

Ah ha!

This is starting to make more sense. This bug is only reproducible via a macro. If I just manually write the code the macro would generate, it compiles fine.

Do you have any pointers on how I might further investigate from this angle?

1 Like
8

It should be possible to figure out based on the debug constraints output whether the same expression is type-checked multiple times. Look for ---Initial constraints for the given expression---.

1 Like
9

That string appears many times in the output I got leading up to the crash. Am I looking for the last occurrence?

10

The last non-indented occurrence should be the one to look for, if you look at the AST it prints, that AST should match a previous the AST for a previous ---Type-checked expression---, modulo the types printed.

1 Like
11

It would also be helpful to reduce the issue down as much as you can. Assuming you have the stack dump, it should give you the frontend invocation. If not, you could re-run swift build with -v to see what the invocation was. With the frontend invocation, you ought to be able to re-run it to reproduce the crash again.

You'll want to take the invocation, and ensure that -primary-file is only done for the file in which you're seeing the crash. Then you ought to be able to start removing unrelated code from the primary file. Once you have a minimal primary file, you should be able to move any code that's being referenced from any secondary files into the primary file, drop the secondary files from the invocation, and then reduce further.

Assuming you're interested in debugging this yourself, you then ought to be able run the invocation with a debug build of the compiler, and set a breakpoint in typeCheckTarget to figure out how/when the expression is being type-checked multiple times.

1 Like
12

I promise I have been trying to reduce this. For what it's worth, the contents of the primary file looks like this:

import Empire

@IndexKeyRecord("a", "b")
struct TestRecord: Hashable {
	let a: String
	let b: UInt
	var c: String
}

The crash occurs during the expansion of this IndexKeyRecord macro. Is it possible for me to reduce this further and keep the macro aspect? That part seems to be a critical component.

13

I was able to significantly reduce what that macro actually does and still reproduce the crash.

The resulting output seems way more managable now. And in fact ---Type-checked expression--- now appears exactly once!

1 Like
14

You ought to at least be able to reduce away the import Empire by inlining the macro declaration for IndexKeyRecord into the file. You won't be able to completely reduce this down to a single-file reproducer given the macro, but if you could reduce the macro implementation down to a single file that can be built standalone (except SwiftSyntax/macro dependencies), that's probably sufficient I'd say.

1 Like
15

That is interesting, the initial expression looks like un-typechecked AST to me, so there may be something else going on here:

---Initial constraints for the given expression---
(call_expr type="Query<Pack{/* shape: $T0 */ repeat $T0}, $T1>" location=@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:6 range=[@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:6 - line:3:19] isolation_crossing="none"
  (type_expr type="Query<Pack{/* shape: $T0 */ repeat $T0}, $T1>.Type" location=@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:6 range=[@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:6 - line:3:6] typerepr="Query")
  (argument_list labels="last:"
    (argument label="last"
      (unresolved_dot_expr type="String" location=@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:18 range=[@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:18 - line:3:18] field="a" function_ref=unapplied
        (declref_expr implicit type="TestRecord.Type" location=@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:18 range=[@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:3:18 - line:3:18] decl="EmpireTests.(file).TestRecord extension.select().self@@__swiftmacro_11EmpireTests10TestRecord08IndexKeyD0fMe_.swift:2:24" function_ref=unapplied)))))
1 Like
16

Ok this was a great suggestion. I have managed to reduce this down significantly.

Input file:

public enum ComparisonOperator<Value: Comparable> {
}

public struct Query<each Component: Comparable, Last: Comparable> {
	public let last: ComparisonOperator<Last>
	public let components: (repeat each Component)

	public init(_ value: repeat each Component, last: ComparisonOperator<Last>) {
		self.components = (repeat each value)
		self.last = last
	}
}

public protocol IndexKeyRecord {
}

@attached(
	extension,
	conformances: IndexKeyRecord,
	names:
		named(select)
)
public macro IndexKeyRecord(
	_ first: StaticString,
	_ remaining: StaticString...
) = #externalMacro(module: "EmpireMacros", type: "IndexKeyRecordMacro")

@IndexKeyRecord("a")
struct TestRecord: Hashable {
	let a: String
}

Macro definition:

import SwiftCompilerPlugin
import SwiftSyntax
import SwiftSyntaxBuilder
import SwiftSyntaxMacros

public struct IndexKeyRecordMacro: ExtensionMacro {
	public static func expansion(
		of node: AttributeSyntax,
		attachedTo declaration: some DeclGroupSyntax,
		providingExtensionsOf type: some TypeSyntaxProtocol,
		conformingTo protocols: [TypeSyntax],
		in context: some MacroExpansionContext
	) throws -> [ExtensionDeclSyntax] {
		return [
			try dataManipulationExtensionDecl(type: type),
		]
	}
}

extension IndexKeyRecordMacro {
	private static func dataManipulationExtensionDecl(
		type: some TypeSyntaxProtocol
	) throws -> ExtensionDeclSyntax {
		let queryArguments = "last: a"
		let funcDecl = try FunctionDeclSyntax(
"""
public static func select() {
Query(\(raw: queryArguments))
}
"""
		)

		return ExtensionDeclSyntax(
			extendedType: type,
			memberBlock: MemberBlockSyntax(
				members: MemberBlockItemListSyntax(
					[MemberBlockItemSyntax(decl: funcDecl)]
				)
			)
		)
	}
}

I really appreciate your help with this. Is this enough of a reduction? Is it help for me to attach these to the github issue?

For what its worth, I attempted to further reduce this by changing:

public let last: ComparisonOperator<Last>

to

public let last: Last

and the crash disappeared.

17

Yup that's sufficient for me to reproduce locally (note you can also remove import SwiftCompilerPlugin), thank you! If you could update the issue with it, that'd be great.

1 Like
18

No, really, thank you. I assume this is pretty far from your top priority, and your help was really amazing. I really apperciate it.