Overloaded operator support. #3796

zygoloid · 2024-03-19T00:16:03Z

Support is added for all overloaded operator interfaces in the current design apart from Assign, which is going to require some more work to properly handle, given that primitive assignment currently has a special implementation for quite a few builtin types.

As we don't have support for generics yet -- in particular, generic interfaces -- there is no support for *With interfaces, but homogenous interfaces such as Add are supported instead.

Factor out building of call expressions so that overloaded operators can generate calls.

Switch a few places from using specific kinds of NodeId to a general NodeId. Because overloaded operators and other things like implicit conversions can result in member access and function calls, those operations can't require a specific kind of NodeId.

Add import support for associated entities, and fix import support for interfaces and symbolic bindings. We now import interfaces in two steps, first importing a forward declaration then a definition, just like we do for classes. For symbolic bindings, we ensure that each BindSymbolicName is imported only once, because its ID is used as its symbolic identity. This is necessary because we (only) support operator interfaces that are defined in an imported Carbon package for now.

The entire contents of check/operator.cpp should probably be rethought. In particular, doing a lot of name lookups on each operator is likely to be bad for performance. But this gets us to the point where overloaded operators are basically working, which seems like a good place to iterate from.

For now, the tests that the individual operators map to the right interfaces are mostly generated by a script, but that's just because I'm expecting a fair bit of churn in how we define the prelude and the impls -- in particular, when we add support for AddWith, we'll need to update all the tests. The plan is to remove the script once things settle down.

Support is added for all overloaded operator interfaces in the current design apart from `Assign`, which is going to require some more work to properly handle, given that primitive assignment currently has a special implementation for quite a few builtin types. As we don't have support for generics yet -- in particular, generic interfaces -- there is no support for `*With` interfaces, but homogenous interfaces such as `Add` are supported instead. Factor out building of call expressions so that overloaded operators can generate calls. Switch a few places from using specific kinds of NodeId to a general NodeId. Because overloaded operators and other things like implicit conversions can result in member access and function calls, those operations can't require a specific kind of NodeId. Add import support for associated entities, and fix import support for interfaces and symbolic bindings. We now import interfaces in two steps, first importing a forward declaration then a definition, just like we do for classes. For symbolic bindings, we ensure that each BindSymbolicName is imported only once, because its ID is used as its symbolic identity. This is necessary because we (only) support operator interfaces that are defined in an imported Carbon package for now. The entire contents of `check/operator.cpp` should probably be rethought. In particular, doing a lot of name lookups on each operator is likely to be bad for performance. But this gets us to the point where overloaded operators are basically working, which seems like a good place to iterate from.

jonmeow

Awesome!

Note regarding name lookup, I've had similar concerns -- I think we need some way of caching operator lookup results. But I'm not sure what the shape of that is. (do we store this on types?)

jonmeow · 2024-03-19T16:22:15Z

.pre-commit-config.yaml

@@ -180,7 +180,7 @@ repos:
            Exceptions. See /LICENSE for license information.
            SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
          - --custom_format
-          - '\.(carbon|proto|ypp)$'
+          - '\.(carbon|proto|ypp)(\.in)?$'


What's a ".in" file? Should this be covered by the PR description?

It looks like you're using it for a script, but it's still Carbon code. We use .def a bit for that in C++; would .def be appropriate here too? Maybe it should be .in.carbon instead of .carbon.in? Perhaps .template or .tmpl instead of .in, to be precise about the use-case?

Switched to .tmpl. (.def suggests C preprocessor input. .in.carbon would get picked up by file_test.)

jonmeow · 2024-03-19T16:35:50Z

toolchain/check/testdata/operators/overloaded/negate.carbon

+
+// --- prelude.carbon
+
+package Carbon api;


Why "Carbon"? If you're trying to create a package name conflict, should this be "Core"? If not, maybe something like "FakePrelude"?

Updated to Core to match general switch from Carbon to Core.

jonmeow · 2024-03-19T16:56:25Z

toolchain/sem_ir/typed_insts.h

@@ -273,8 +273,7 @@ struct BoolLiteral {
 // `self` parameter, such as `object.MethodName`.
 struct BoundMethod {
  static constexpr auto Kind =
-      InstKind::BoundMethod.Define<Parse::AnyMemberAccessExprId>(


Are you removing all uses of AnyMemberAccessExprId? Should the definition be deleted?

jonmeow · 2024-03-19T16:57:09Z

toolchain/check/testdata/operators/overloaded/make_tests.sh

+make_test() {
+  HEADER="// This file was generated from $4. Run make_tests.sh to regenerate."
+  sed "s,INTERFACE,$1,g; s,OP,$2,g; s,HEADER,$HEADER," $4 > $3.carbon
+}


I have concerns about this script; putting a script in testdata feels like it hides it from developers (I would only expect data files here, not code). Additionally, it adds another manual execution step separate from autoupdate. Shell scripts are additionally a barrier if we try to support Windows -- I would suggest a preference for Python, as it has better cross-platform compatibility.

What alternatives had you considered? A few I might suggest if you haven't considered them already:

Use a genrule to generate boilerplate files.

file_test should be okay taking any BUILD output as input, so I think this would mostly work.

You'd probably and to specify ARGS to exclude the IR (i.e., to avoid update issues, making this just a test that it compiles)

genrules can still be written to be platform-specific, so please be careful about this -- I think we currently only have genrules in explorer code.

Write a traditional unit test that generates content and runs the driver on it directly.

This feels a little related to the genrule, although shifting away from file_test a little more. End results are probably the same.

Add a utility in file_test for templating.

A way to still have autoupdate support.

Just to note, discussed this offline: agreed to a TODO to remove the script, with the expectation that the [actual] prelude would lead to these being pretty small and not worth autoupdate anymore.

[avoiding various template approaches to avoid abstraction of test files and golden output]

Per discussion, added a TODO to say the script is short-term and should be removed once things stabilize here.

toolchain/check/operator.h

toolchain/check/handle_operator.cpp

jonmeow · 2024-03-19T17:18:16Z

toolchain/check/operator.cpp

+    return SemIR::NameScopeId::Invalid;
+  }
+
+  package_id = context.constant_values().Get(package_id).inst_id();


Why does this re-resolve through the constant? (a comment might help)

Comment added.

jonmeow · 2024-03-19T17:19:55Z

toolchain/check/operator.cpp

+
+namespace Carbon::Check {
+
+// Returns the scope of the Carbon package, or Invalid if it's not found.


Why "Carbon" versus "Core"? Wasn't "Core" the outcome of #2113 (comment)?

Ah right. Switched to Core throughout.

We should have a proposal for that, and update the documentation to match :-)

jonmeow · 2024-03-19T17:20:37Z

toolchain/check/operator.cpp

+    return SemIR::InstId::Invalid;
+  }
+
+  op_id = context.constant_values().Get(op_id).inst_id();


Similar to package_id comment, this feels odd.

Comment added.

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/check/handle_operator.cpp

toolchain/check/operator.h

Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>

Add TODO to remove script. Minimize size of generated tests.

github-actions bot requested a review from geoffromer March 19, 2024 00:16

github-actions bot added the toolchain label Mar 19, 2024

jonmeow reviewed Mar 19, 2024

View reviewed changes

Apply suggestions from code review

3ee7364

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

CarbonInfraBot reviewed Mar 19, 2024

View reviewed changes

toolchain/check/handle_operator.cpp Outdated Show resolved Hide resolved

toolchain/check/handle_operator.cpp Outdated Show resolved Hide resolved

toolchain/check/operator.h Outdated Show resolved Hide resolved

toolchain/check/operator.h Outdated Show resolved Hide resolved

zygoloid and others added 6 commits March 19, 2024 10:59

Apply suggestions from code review

a172817

Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>

Refactor interface import to match class import a bit better.

bbe57f5

Rename package Carbon -> Core, per carbon-language#2113.

0dd01fa

Update import tests now we can import associated entities.

67d30b5

Comment.

e98e8ac

Remove unused ID types.

d2ebc9b

jonmeow approved these changes Mar 19, 2024

View reviewed changes

Rename .in -> .tmpl.

bbab251

Add TODO to remove script. Minimize size of generated tests.

zygoloid enabled auto-merge March 19, 2024 18:49

Fix crash in fuzzer test exposed by -1 no longer aborting compilation.

64bf90d

zygoloid added this pull request to the merge queue Mar 19, 2024

Merged via the queue into carbon-language:trunk with commit cf361a8 Mar 19, 2024
7 checks passed

zygoloid deleted the toolchain-operators branch March 19, 2024 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overloaded operator support. #3796

Overloaded operator support. #3796

zygoloid commented Mar 19, 2024 •

edited

Loading

jonmeow left a comment

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

jonmeow Mar 19, 2024 •

edited

Loading

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024

jonmeow Mar 19, 2024

zygoloid Mar 19, 2024


		namespace Carbon::Check {

		// Returns the scope of the Carbon package, or Invalid if it's not found.

Overloaded operator support. #3796

Overloaded operator support. #3796

Conversation

zygoloid commented Mar 19, 2024 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid commented Mar 19, 2024 •

edited

Loading

jonmeow Mar 19, 2024 •

edited

Loading