LLVM Crash with using a private type as default value outside of the package

[sgebbie]

When declaring a type as private to a package, but then using that value as the default value poncy crashes (SIGSEGV) within llvm with the following message:

• PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Code to reproduce the error

// inside the "useful" package

primitive _PrivateDefault

actor Useful[A: Any val]
  fun tag config(value: (A | _PrivateDefault) = _PrivateDefault): Useful[A] => this

// inside "main"

use "useful"

primitive This
primitive That

type Stuff is (This | That)

actor Main
  new create(env: Env) =>
    let u = Useful[Stuff].config()

Commentary indicating the possible source of the problem

See: method_def in check_partial_function_call ends up being null.

  // Look up the original method definition for this method call.
  deferred_reification_t* method_def = lookup(opt, receiver, ast_type(receiver),
    ast_name(method));
  ast_t* method_ast = method_def->ast;
  deferred_reify_free(method_def);

See also

• https://github.com/ponylang/ponyc/blob/main/src/libponyc/verify/call.c#L31
• Zulip chat: https://ponylang.zulipchat.com/#narrow/stream/189934-general/topic/llvm.20crash/near/284939136

[ergl]

The code in OP fails even if one doesn't use a union type when calling the method. Here's a more minimal example:

// In the "lib" pacakge

primitive _Private

primitive Public
  fun apply[T](v: (T | _Private) = _Private): None => None

// In main
use lib = "lib"

actor Main
  new create(env: Env) =>
    let p = lib.Public.apply[U8]()
    env.out.print(p.string())

---

The problem here is that lookup, by default, doesn't allow one to include symbols that are package-private. See here:

ponyc/src/libponyc/type/lookup.c

Lines 66 to 79 in ff5e8c2

	if(is_name_private(type_name) && (from != NULL) && (opt != NULL)
	&& !allow_private)
	{
	if(ast_nearest(def, TK_PACKAGE) != t->frame->package)
	{
	if(errors)
	{
	ast_error(opt->check.errors, from, "can't lookup fields or methods "
	"on private types from other packages");
	}
	return NULL;
	}
	}

Since the _PrivateDefault type is private, lookup will return NULL, which leads to a segfault. Solution here, depending on what the correct behaviour should be:

1. If the code in OP should be accepted, then we should use lookup_try, which allows one to set allow_private to true.

2. If the code in OP shouldn't be accepted, then we should correctly handle a NULL return. In our case, lookup already sets an appropriate error message, so we should just print it to the user and return false from check_partial_function_call (which, in spite of the name, is called for any function, not just for partial functions)

[jemc]

This was discussed in the last ~20m or so of the sync call.

Today in Pony, we allow private types to be in the parameter types or return type of a public function signature. It may be considered by some to be "bad practice" to do so, but it is allowed. So for consistency with that, I think we'd have to also allow the default argument expression to name a private type (as was being attempted in this example).

However, one of the things I have been thinking about lately relates to the automatic detection of breaking changes in type signatures, for tooling to assist with semantic versioning. The challenge there is that if we allow private types to be the return type of a public function, then all public functions of the private type become part of the public API contract to be upheld with respect to semantic versioning.

So with that in mind, I think I'd be in favor of a breaking change to Pony language to disallow private types to be named in the public method signatures of a public type.

Gordon and red were both surprised that Pony allows this today and agreed with the idea of disallowing it.

[sgebbie]

This was discussed in the last ~20m or so of the sync call.

Today in Pony, we allow private types to be in the parameter types or return type of a public function signature. It may be considered by some to be "bad practice" to do so, but it is allowed. So for consistency with that, I think we'd have to also allow the default argument expression to name a private type (as was being attempted in this example).

However, one of the things I have been thinking about lately relates to the automatic detection of breaking changes in type signatures, for tooling to assist with semantic versioning. The challenge there is that if we allow private types to be the return type of a public function, then all public functions of the private type become part of the public API contract to be upheld with respect to semantic versioning.

So with that in mind, I think I'd be in favor of a breaking change to Pony language to disallow private types to be named in the public method signatures of a public type.

Gordon and red were both surprised that Pony allows this today and agreed with the idea of disallowing it.

While allowing private values as a default did provide a convenient way of providing a "hidden default," I have to admit that in the back of my mind it did feel like it wasn't a totally legitimate use of the language.

So, I am inclined to agree that it should be flagged as a compile time error.

[SeanTAllen]

So the allowing of private types like this was core to the RFC to change collections iterators so its a well trod path at the moment for many in terms of it being discussed a lot. Both on at least 1 sync call, in Zulip, and as the basis for a very discussed RFC. I am not in favor of changing that behaviour without an RFC to consider the ramifications.

I would like for us to say that we will fix this bug as it stands without touching anything related to private visibility outside of a package if explicitly provided. That should only be done if an RFC is accepted.

I'd like to get others to agree first though. I know we have a user who is interested in fixing this issue so I'd like to come to some agreement as to what we do next.

[jemc]

Fixing the current bug as it exists makes sense to me, as long as the investment to do so is not too high.

[jemc]

We discussed this more at today's sync.

Sean has spent time looking at how semver tooling would need to apply in the light of the current Pony behavior of allowing private types to be returned in a public method/field - such types would have to be marked as being "exported" by the package despite being private, and thus their public methods/fields would also be considered package-public. This would need to be applied transitively in order to determine which private types have public methods/fields that are package-public.

This rule would be correct for current Pony under the definition of correctness that says "a change to a package is not a breaking change if it is not possible to write a downstream program that could be broken by that change".

Also, I noticed that even though this conversation has drifted toward disallowing this use case for semantic versioning tooling simplicity reasons, the use case in this ticket (accepting a private parameter type and/or using a private type name in the default expression value) does not actually have the effect of "exporting" that private type, using the definition we discussed above - it would only be considered "exported" if it was in the return type.

[stefandd]

Fixing the current bug as it exists makes sense to me, as long as the investment to do so is not too high.

The problem is as @ergl pointed out there are 2 ways to fix: one allowing this use of a private parameter type and the other disallowing it and displaying an error message. Has the sync produced a concensus re: this?

[SeanTAllen]

The inability to use the private parameter is a bug and will be fixed. It shouldn't be a compiler error as nothing is wrong with the example code. It's valid Pony.

[stefandd]

@SeanTAllen What about the poor code in lookup.c (potential compiler crash) I pointed out to you? Should this be fixed separately? Or should I monitor this issue and point it out when proposed changes have been PR-ed?

[SeanTAllen]

Please open issues for anything that isn't specifically the fix to this issue or PRs if you want.