Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function #1256

pk19604014 · 2022-05-12T14:27:02Z

…n parameter and return type before calling InterpExp() on them

jonmeow

I'm not clear that this is the right fix: if InterpExp needs to always type-check the expression first, shouldn't type_checker have a wrapper for InterpExp that does that, rather than expecting each place that calls InterpExp to typecheck explicitly?

jonmeow · 2022-05-12T17:48:16Z

As an example, TupleValue seems to call InterpExp without calling TypeCheckExp, maybe that could have this same bug? It feels like a central solution that addresses all these cases equally may be safer.

pk19604014 · 2022-05-12T18:14:22Z

As an example, TupleValue seems to call InterpExp without calling TypeCheckExp, maybe that could have this same bug? It feels like a central solution that addresses all these cases equally may be safer.

For this patch, I temporarily added a (void)e.static_type() call to InterExp() and InterpPattern() to verify there were no more crashes surfaced by existing tests (and can add a permanent check based on CheckHasStaticType() Geoff suggested as a follow-up).

I agree a wrapper might be good, but some places like the logic for FieldAccessExpression (around line 744 in type_checker.cpp) sometimes only calls TypeCheckExp() and not InterpExp() depending on expression type, so the wrapper won't be used 'pervasively' -- if that sounds good I can change 'obvious' TypeCheck+Interp' sequences to use a wrapper.

zygoloid · 2022-05-12T21:03:53Z

I'm pretty suspicious of anything that type-checks and then immediately evaluates an expression -- I would expect there to be a step in between that checks that the type of the expression is what we were expecting and, if not, attempts implicit conversion to that type. Notionally what this function type handling code should be doing is type-checking the expression, then implicitly converting it to type Type, then evaluating it, rather than type-checking, then evaluating, then checking if the result is a type value. (The difference between these approaches will become visible once we support user-defined implicit conversions.)

It might be premature to factor out a wrapper before those conversions are in place; we might find the call sites end up a bit less uniform once that's done. (Or we might not, I'm not sure.)

jonmeow · 2022-05-12T21:20:12Z

@zygoloid There's a thread on #explorer, if you want to disagree can you take it up there so we're discussing in one place?

Trying to figure out what zygoloid wants

zygoloid · 2022-05-12T23:57:17Z

After discussion with @jonmeow I think it would be reasonable to factor out a function for type-checking expressions that are expected to produce types. For example, a function TypeCheckTypeOperand that takes a type and calls:

TypeCheckExp
InterpExp
ExpectIsType or ExpectIsConcreteType on the result

(And at some later point we can add an implicit conversion to the Type type between the first two steps.) That covers at least about half of the calls to InterpExp in the type-checker and seems like a good path forward to me.

Looking through the calls to InterpExp, it looks like there are quite a few that should fall into the above pattern, but don't: that is, they expect a type but don't actually check that they got one:

Type-checking a GenericBinding (Eg, fn F[a:! 42](); is currently incorrectly accepted.)
Type-checking the type in an impl (Eg, impl "hello" as Interface {} is currently incorrectly accepted.)
Type-checking a choice in a ChoiceDeclaration (Eg, choice C { X(5) } is currently incorrectly accepted.)
Changing those to TypeCheckTypeOperand should fix those bugs.

pk19604014 · 2022-05-13T13:29:09Z

After discussion with @jonmeow I think it would be reasonable to factor out a function for type-checking expressions that are expected to produce types. For example, a function TypeCheckTypeOperand that takes a type and calls:

TypeCheckExp

InterpExp

ExpectIsType or ExpectIsConcreteType on the result

(And at some later point we can add an implicit conversion to the Type type between the first two steps.) That covers at least about half of the calls to InterpExp in the type-checker and seems like a good path forward to me.

Looking through the calls to InterpExp, it looks like there are quite a few that should fall into the above pattern, but don't: that is, they expect a type but don't actually check that they got one:

Type-checking a GenericBinding (Eg, fn F[a:! 42](); is currently incorrectly accepted.)

Type-checking the type in an impl (Eg, impl "hello" as Interface {} is currently incorrectly accepted.)

Type-checking a choice in a ChoiceDeclaration (Eg, choice C { X(5) } is currently incorrectly accepted.)
Changing those to TypeCheckTypeOperand should fix those bugs.

Would it be ok to merge the current PR as is to fix the immediate crashing bug, and do the suggested refactoring/improvement changes as a follow-up (as these changes are less trivial now)?

zygoloid

Would it be ok to merge the current PR as is to fix the immediate crashing bug, and do the suggested refactoring/improvement changes as a follow-up (as these changes are less trivial now)?

Either way works for me; I'll leave this for @jonmeow to decide.

jonmeow · 2022-05-16T16:30:55Z

I'd rather it be done here -- let me know if you need more explanation on why, but @pk19604014 I think that VariableDeclaration should probably be verifying what InterpExp is returning, and @zygoloid's suggestion is both consistent with my earlier request and would help make that VariableDeclaration difference more obvious. The present state feels like an incomplete fix to me, which while it may stop crashing, doesn't feel like the right outcome. Also, the change to a helper function still feels quite trivial to me.

…into fuzz_231844390

jsiek · 2022-05-18T17:47:36Z

I like the suggestion of @zygoloid regarding creating a helper function for type expressions (TypeCheckTypeOperand).

pk19604014 · 2022-05-19T21:18:24Z

I'd rather it be done here -- let me know if you need more explanation on why, but @pk19604014 I think that VariableDeclaration should probably be verifying what InterpExp is returning, and @zygoloid's suggestion is both consistent with my earlier request and would help make that VariableDeclaration difference more obvious. The present state feels like an incomplete fix to me, which while it may stop crashing, doesn't feel like the right outcome. Also, the change to a helper function still feels quite trivial to me.

Ok done PTAL

zygoloid · 2022-05-20T00:55:25Z

explorer/testdata/impl/fail_impl_as_parameterized.carbon

+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/impl/fail_impl_as_parameterized.carbon:[[@LINE+1]]: expected constraint after `as`, found value of type String
+external impl i32 as String {}


This test was testing the case where the thing after as is the name of a parameterized interface that's missing its parameters. I think we still want to test that case explicitly, even if it covers the same code paths as this case right now; can we add this file as a new test and keep the existing test as-is?

zygoloid · 2022-05-20T01:04:26Z

explorer/interpreter/type_checker.cpp

-            auto value, InterpExp(&arg.expression(), arena_, trace_stream_));
-        CARBON_RETURN_IF_ERROR(
-            ExpectIsConcreteType(arg.expression().source_loc(), value));
+        CARBON_RETURN_IF_ERROR(TypeCheckTypeExp(&arg.expression(), impl_scope));


I think that these cases that are ignoring the return value shouldn't be interpreting the type expression at all. This will result in quadratic runtime for a case such as {.a: {.a: {.a: ...}}} because each level of struct type will repeat the evaluation of the inner struct literal. I've verified the runtime of this grows superlinearly in practice:

~45ms for 50 levels of nesting

~80ms for 100 levels of nesting

~200ms for 200 levels of nesting

~680ms for 400 levels of nesting

I guess this is not really fixable until we switch away from ExpectIsType to implicitly converting to Type, though, so let's not block this patch on a fix for that.

zygoloid

Looks good to me with the requested test fix.

zygoloid · 2022-05-23T23:26:56Z

Merging now; I'll make the requested test fix myself as a separate PR.

This change updated a test to use a different example to make sure a diagnostic was still being exercised, but as a consequence lost test coverage of the situation the test was originally intended to cover. Add back the old test and put the new test into a different file.

…r type-checking logic to use the new function (#1256)

This change updated a test to use a different example to make sure a diagnostic was still being exercised, but as a consequence lost test coverage of the situation the test was originally intended to cover. Add back the old test and put the new test into a different file.

Changed function type literal declaration logic to type-check functio…

915377c

…n parameter and return type before calling InterpExp() on them

pk19604014 marked this pull request as ready for review May 12, 2022 14:46

pk19604014 requested a review from a team as a code owner May 12, 2022 14:46

jonmeow previously requested changes May 12, 2022

View reviewed changes

pk19604014 requested a review from jonmeow May 12, 2022 18:14

jonmeow requested review from zygoloid and removed request for jonmeow May 12, 2022 23:14

pk19604014 requested a review from jonmeow May 13, 2022 13:29

zygoloid reviewed May 13, 2022

View reviewed changes

Merge branch 'trunk' of https://github.com/carbon-language/carbon-lang …

b02f6ff

…into fuzz_231844390

pk19604014 added 2 commits May 19, 2022 14:52

merged

f6acec7

Started converting the code to use TypeCheckConcreteTypeExp()

2204042

zygoloid mentioned this pull request May 19, 2022

Split TypeCheckExp into recursive and non-recursive steps #1277

Closed

pk19604014 added 2 commits May 19, 2022 16:47

converted a few more usages to TypeCheck[Concrete]Type

bee3bc2

converted more usages to TypeCheckTypeExp

51f418f

pk19604014 requested a review from zygoloid May 19, 2022 21:18

pk19604014 changed the title ~~Changed function type literal declaration logic to type-check function parameter and return type before calling InterpExp() on them~~ Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function May 19, 2022

zygoloid reviewed May 20, 2022

View reviewed changes

zygoloid approved these changes May 20, 2022

View reviewed changes

zygoloid merged commit 5ca3dd8 into carbon-language:trunk May 23, 2022

pk19604014 mentioned this pull request May 24, 2022

Remove broken fuzzing input, presumably added accidentally in #1256. #1288

Closed

chandlerc pushed a commit that referenced this pull request Jun 28, 2022

Added TypeCheckTypeExp() and changed function type literal and simila…

f0a34e4

…r type-checking logic to use the new function (#1256)

chandlerc added the explorer Action items related to Carbon explorer code label Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function #1256

Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function #1256

pk19604014 commented May 12, 2022

jonmeow left a comment

jonmeow commented May 12, 2022

pk19604014 commented May 12, 2022

zygoloid commented May 12, 2022

jonmeow commented May 12, 2022

zygoloid commented May 12, 2022

pk19604014 commented May 13, 2022

zygoloid left a comment

jonmeow commented May 16, 2022 •

edited

Loading

jsiek commented May 18, 2022

pk19604014 commented May 19, 2022

zygoloid May 20, 2022

zygoloid May 20, 2022

zygoloid left a comment

zygoloid commented May 23, 2022

		// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/impl/fail_impl_as_parameterized.carbon:[[@LINE+1]]: expected constraint after `as`, found value of type String
		external impl i32 as String {}

Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function #1256

Added TypeCheckTypeExp() and changed function type literal and similar type-checking logic to use the new function #1256

Conversation

pk19604014 commented May 12, 2022

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow commented May 12, 2022

pk19604014 commented May 12, 2022

zygoloid commented May 12, 2022

jonmeow commented May 12, 2022

zygoloid commented May 12, 2022

pk19604014 commented May 13, 2022

zygoloid left a comment

Choose a reason for hiding this comment

jonmeow commented May 16, 2022 • edited Loading

jsiek commented May 18, 2022

pk19604014 commented May 19, 2022

zygoloid May 20, 2022

Choose a reason for hiding this comment

zygoloid May 20, 2022

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

zygoloid commented May 23, 2022

jonmeow commented May 16, 2022 •

edited

Loading