change tuple indexing #4186

brymer-meneses · 2024-08-03T06:02:41Z

this changes the tuple index from tuple[0] to tuple.0 in accordance with the accepted propsal #3646

josh11b

Some initial feedback. I'll look again once these are addressed.

toolchain/check/testdata/index/fail_tuple_large_index.carbon

toolchain/check/handle_name.cpp

toolchain/check/member_access.cpp

toolchain/check/testdata/index/index_not_literal.carbon

toolchain/parse/handle_period.cpp

josh11b · 2024-08-05T22:19:48Z

toolchain/check/member_access.cpp

+        CARBON_DIAGNOSTIC(
+            TupleWrongIndexSyntax, Error,
+            "Type `{0}` does not support array indexing. Did you mean tuple.0?",
+            SemIR::TypeId);


Should add a test that verifies that this diagnostic is being produced correctly. Also, isn't this the opposite case, where you are using tuple indexing on a non-tuple value?

Also, isn't this the opposite case, where you are using tuple indexing on a non-tuple value?

My bad, for some reason I was thinking of tuple[0]. Should we add a note telling the user the right way to index a tuple?

That would be great, but I wouldn't say it is required.

toolchain/check/member_access.cpp

toolchain/sem_ir/typed_insts.h

brymer-meneses · 2024-08-06T08:09:17Z

I think everything should be resolved by now, except the parts wherein I am confused on what to do.

brymer-meneses · 2024-08-07T13:02:45Z

Also, apologies if I'm unnecessarily pinging you by re-requesting a review even if you said that you'd review this once everything gets resolved.

jonmeow

Thanks, this looks really good! Josh had asked me to take a look, and I think the logic looks correct. I did see some spots though where I think it may be helpful to adjust API use, just subtle toolchain things.

jonmeow · 2024-08-07T17:43:10Z

toolchain/check/testdata/tuples/fail_non_tuple_index.carbon

+  // CHECK:STDERR: fail_non_tuple_index.carbon:[[@LINE+3]]:20: ERROR: Type `[i32; 1]` does not support tuple indexing. Only tuples can be indexed that way.
+  // CHECK:STDERR:   var first: i32 = non_tuple.0;
+  // CHECK:STDERR:                    ^~~~~~~~~~~
+  var first: i32 = non_tuple.0;


It looks like the other tuple indexing tests are under testdata/index. Might that be a better place for this test?

I didn't notice that there was a similar test named toolchain/check/testdata/fail_non_tuple_access. I think I'll just merge this with that.

Had you meant to remove this?

toolchain/lower/testdata/index/array_element_access.carbon

toolchain/parse/testdata/basics/fail_invalid_designators.carbon

toolchain/check/testdata/index/fail_negative_indexing.carbon

toolchain/parse/handle_period.cpp

toolchain/sem_ir/typed_insts.h

toolchain/check/member_access.h

toolchain/check/handle_name.cpp

toolchain/check/member_access.cpp

jonmeow · 2024-08-07T20:56:49Z

As a sidenote (not something I'm requesting for this PR), I'm thinking we may want to split up the index directory to reflect the logic split. Maybe leaving the IndexWith-y things in index, and moving the tuple indexing out to something like tuples/access. (which, is now making me notice that odd plural of "tuples": #4200)

This is partly because I feel odd seeing so many files with either array or tuple in the name, it seems like it'd be nice to have that implicit from the directory name. You can do this here if you want, but I'm happy to pick it up after you're merged since it feels like scope creep here.

brymer-meneses · 2024-08-08T03:50:53Z

You can do this here if you want, but I'm happy to pick it up after you're merged since it feels like scope creep here.

I'll happily do this in a separate PR, as I do not want to clutter this PR by syncing to trunk (to have #4200).

brymer-meneses · 2024-08-08T07:16:06Z

On a related note, do you think we should rename tuple index with tuple access across the repo? That would mean renaming a lot of things ( SemIR, typed_nodes.h) and stuff.

jonmeow · 2024-08-08T19:59:22Z

On a related note, do you think we should rename tuple index with tuple access across the repo? That would mean renaming a lot of things ( SemIR, typed_nodes.h) and stuff.

Asking for thoughts on discord. Maybe best to split out any rename.

jonmeow · 2024-08-08T21:21:52Z

Per discussion, we'll want to consolidate TupleIndex into the already-existing TupleAccess. They're essentially equivalent, and that'd align with how StructAccess works: using the same instruction for both implicit and explicit access. [ed: note this too might be better in a different PR, but I'm not sure how big the delta would be]

jonmeow

Thanks for all the changes! Basically looks good, but I think you might've missed deleting the fail_non_tuple_index.carbon. Since there's fail_non_tuple_access (as you observed), can you delete that and I'll merge?

jonmeow · 2024-08-14T23:47:26Z

toolchain/check/testdata/tuples/fail_non_tuple_index.carbon

+  // CHECK:STDERR: fail_non_tuple_index.carbon:[[@LINE+3]]:20: ERROR: Type `[i32; 1]` does not support tuple indexing. Only tuples can be indexed that way.
+  // CHECK:STDERR:   var first: i32 = non_tuple.0;
+  // CHECK:STDERR:                    ^~~~~~~~~~~
+  var first: i32 = non_tuple.0;


Had you meant to remove this?

jonmeow · 2024-08-14T23:51:52Z

toolchain/parse/testdata/index/tuple_index.carbon

+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/parse/testdata/tuple/index.carbon
+
+let tuple: (i32, i32) = (5, 5);


Suggested change

let tuple: (i32, i32) = (5, 5);

This is really small (I'm mainly mentioning it because of fail_non_tuple_index.carbon), but I think it's helpful to both emphasize the tuple.0 parse tree, and as a hint that this line isn't needed (name lookup doesn't occur during parse). So the parse tree below ends up a lot shorter, making it easier to see the syntax under test.

jonmeow · 2024-08-14T23:56:10Z

Thanks for all the changes! Basically looks good, but I think you might've missed deleting the fail_non_tuple_index.carbon. Since there's fail_non_tuple_access (as you observed), can you delete that and I'll merge?

Actually, it looks like things have gotten slightly out of date due to a LLVM IR change (#4212 looks like the source of conflicts). I'm happy to have you merge/autoupdate, or I can take care of that (let me know how you'd prefer).

When forming a specific (previously called a generic instance), evaluate the eval block of the generic to determine the values of any constants used in that specific. The majority of the work here is updating eval.cpp so that it can use the results of prior evaluations in the same block when computing later values. Include the computed results in the formatted SemIR output. --------- Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

…rbon-language#4131)

When evaluating in a generic context, a constant with a symbolic type might evaluate to a constant with a concrete type (or a more specific symbolic type). This can't actually happen yet given the current state of the toolchain, as far as I can determine, so this is more just a refactoring for now, but will be relied upon by future generics work. --------- Co-authored-by: Jon Ross-Perkins <jperkins@google.com> Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>

…anguage#4136) The output is really basic, I'm just adding this to help track how memory is allocated. ``` --- filename: 'check/testdata/expr_category/in_place_tuple_init.carbon' source_: used_bytes: 8057 reserved_bytes: 8057 tokens_.allocator_: used_bytes: 0 reserved_bytes: 0 tokens_.token_infos_: used_bytes: 1040 reserved_bytes: 2032 (eliding) value_stores_.string_literals_.set_: used_bytes: 320 reserved_bytes: 320 Total: used_bytes: 20609 reserved_bytes: 29437 ... ``` --------- Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

…arbon-language#4137) Querying a local constant with an invalid instruction triggers a crash, this will prevent self_param_id to be used unless it is defined. Closes carbon-language#4071 and carbon-language#4080 This will only fix the crash. The scenario where a declaration-only interface is imported then defined still gives an error message, but it won't crash this time.

…re them to be fully defined. (carbon-language#4139) When referring to a constant within a specific, such as a field of a generic class, use that specific version of the constant's value.

…nguage#4142) Most of this is enabled by default, but there is some that needs an explicit flag. Avoiding `-Wnon-virtual-dtor` itself for now because that warning can require changes that carry overhead such as having an extra, unused destructor entry in the vtable. Hopefully we don't have too many folks who need our code to be `-Wnon-virtual-dtor` clean.

…-language#4133) This improve the toolchain's performance by about 10%. It will also allow us to leverage TCMalloc's extensions to do heap profiling and get other information about how efficiently we're using the heap. Note that currently this causes all of our builds to produce a warning due to an issue with `rules_python` and multiple modules registering python toolchains: bazelbuild/rules_python#1818 This is also only enabled on Linux as there is no support for other OSes at the moment.

…bon-language#4141)

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

also fix the bug the wherein the diagnostic `TupleIndexOnANonTupleType` was not showing

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

…ail_non_tuple_access.carbon

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

brymer-meneses · 2024-08-15T06:44:12Z

I think, I might have messed things up when trying to sync.

This changes the tuple index from tuple[0] to tuple.0 in accordance with the accepted propsal #3646 I messed up syncing to trunk on my original PR #4186, that's why I'm starting on a blank state. Please let me know if I missed incorporating a change from my prior PR.

brymer-meneses added 4 commits July 16, 2024 04:12

docs: add a note on setting up clangd

0babda5

docs: add a note on setting up clangd

db6b327

apply suggestions

8aa7919

forgot a semicolon

67b6fd3

github-actions bot requested a review from josh11b August 3, 2024 06:02

github-actions bot added the toolchain label Aug 3, 2024

josh11b reviewed Aug 5, 2024

View reviewed changes

brymer-meneses commented Aug 6, 2024

View reviewed changes

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

brymer-meneses commented Aug 6, 2024

View reviewed changes

toolchain/sem_ir/typed_insts.h Show resolved Hide resolved

brymer-meneses requested a review from josh11b August 7, 2024 06:52

jonmeow reviewed Aug 7, 2024

View reviewed changes

brymer-meneses requested a review from jonmeow August 14, 2024 16:53

jonmeow approved these changes Aug 14, 2024

View reviewed changes

zygoloid and others added 9 commits August 15, 2024 12:56

Build an evaluation block for the definition region of a generic. (ca…

e946c68

…rbon-language#4131)

Substitute into generic class and interface definitions when we requi…

0ca907f

…re them to be fully defined. (carbon-language#4139) When referring to a constant within a specific, such as a field of a generic class, use that specific version of the constant's value.

Mark some leaf classes final to suppress -Wnon-virtual-dtor. (car…

f7b1793

…bon-language#4141)

brymer-meneses and others added 23 commits August 15, 2024 12:57

Update toolchain/check/handle_name.cpp

53e7902

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

Update toolchain/check/handle_name.cpp

8626e5d

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

Update toolchain/check/member_access.cpp

1332dd2

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

Update toolchain/parse/handle_period.cpp

c5884f3

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

Update toolchain/check/member_access.cpp

8bd014d

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

Update toolchain/lower/testdata/index/tuple_return_value_access.carbon

e83f6cf

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

handle tuple indexing of paren expressions

2575124

update tests

3dcaff0

add fail_non_tuple_index.carbon

d4ebcb4

also fix the bug the wherein the diagnostic `TupleIndexOnANonTupleType` was not showing

add comment to PerformTupleIndex

f9247c3

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

add missing comments

a52081b

refactor toolchain/check/member_access.cpp

ab7d674

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

move IntLiteral

eae1ba0

Update toolchain/check/member_access.cpp

c2e773d

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

apply suggestion

28e9114

apply suggestion

a8a4c6a

consolidate tuples/fail_non_tuple_index.carbon with `tuples/index/f…

91ceae5

…ail_non_tuple_access.carbon

Update toolchain/lower/testdata/index/array_element_access.carbon

ec26bec

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

apply test changes

e54d9e1

add tuple negative indexing

4209a32

testdata changes

ad13f30

testdata changes

56ec41f

testdata changes

91d3545

brymer-meneses force-pushed the tuple-indexing branch 2 times, most recently from 593a2b0 to 91d3545 Compare August 15, 2024 06:37

brymer-meneses closed this Aug 15, 2024

brymer-meneses mentioned this pull request Aug 15, 2024

change tuple index #4218

Merged

brymer-meneses mentioned this pull request Aug 17, 2024

move tuple testdata from index/ to tuple/access #4226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change tuple indexing #4186

change tuple indexing #4186

brymer-meneses commented Aug 3, 2024

josh11b left a comment

josh11b Aug 5, 2024

brymer-meneses Aug 6, 2024 •

edited

Loading

josh11b Aug 7, 2024

brymer-meneses commented Aug 6, 2024 •

edited

Loading

brymer-meneses commented Aug 7, 2024

jonmeow left a comment

jonmeow Aug 7, 2024

brymer-meneses Aug 8, 2024 •

edited

Loading

jonmeow Aug 14, 2024

jonmeow commented Aug 7, 2024 •

edited

Loading

brymer-meneses commented Aug 8, 2024 •

edited

Loading

brymer-meneses commented Aug 8, 2024 •

edited

Loading

jonmeow commented Aug 8, 2024

jonmeow commented Aug 8, 2024 •

edited

Loading

jonmeow left a comment

jonmeow Aug 14, 2024

jonmeow Aug 14, 2024

jonmeow commented Aug 14, 2024

brymer-meneses commented Aug 15, 2024

change tuple indexing #4186

change tuple indexing #4186

Conversation

brymer-meneses commented Aug 3, 2024

josh11b left a comment

Choose a reason for hiding this comment

josh11b Aug 5, 2024

Choose a reason for hiding this comment

brymer-meneses Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

josh11b Aug 7, 2024

Choose a reason for hiding this comment

brymer-meneses commented Aug 6, 2024 • edited Loading

brymer-meneses commented Aug 7, 2024

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow Aug 7, 2024

Choose a reason for hiding this comment

brymer-meneses Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

jonmeow Aug 14, 2024

Choose a reason for hiding this comment

jonmeow commented Aug 7, 2024 • edited Loading

brymer-meneses commented Aug 8, 2024 • edited Loading

brymer-meneses commented Aug 8, 2024 • edited Loading

jonmeow commented Aug 8, 2024

jonmeow commented Aug 8, 2024 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow Aug 14, 2024

Choose a reason for hiding this comment

jonmeow Aug 14, 2024

Choose a reason for hiding this comment

jonmeow commented Aug 14, 2024

brymer-meneses commented Aug 15, 2024

brymer-meneses Aug 6, 2024 •

edited

Loading

brymer-meneses commented Aug 6, 2024 •

edited

Loading

brymer-meneses Aug 8, 2024 •

edited

Loading

jonmeow commented Aug 7, 2024 •

edited

Loading

brymer-meneses commented Aug 8, 2024 •

edited

Loading

brymer-meneses commented Aug 8, 2024 •

edited

Loading

jonmeow commented Aug 8, 2024 •

edited

Loading