Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checker: infer map array values based on first value of map #19411

Closed
wants to merge 4 commits into from

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Sep 22, 2023

Closes #19253

🤖 Generated by Copilot at 3b8ac2a

This pull request enhances the type checking and casting of maps with array values in V. It modifies vlib/v/checker/containers.v and adds a new test file vlib/v/tests/map_array_cast_value_acc_first_test.v.

🤖 Generated by Copilot at 3b8ac2a

  • Add a variable expecting_array_map to check if the map value type is an array (link)
  • Make val_type_sym mutable to allow modifying the map value type if it is an array (link)
  • Handle the case of casting map values to arrays of the same element type as the first value (link)
  • Add a test file map_array_cast_value_acc_first_test.v to verify the functionality of the new feature (link)

@Delta456 Delta456 marked this pull request as draft September 22, 2023 09:32
@Delta456 Delta456 marked this pull request as ready for review September 22, 2023 10:13
@joe-conigliaro
Copy link
Member

joe-conigliaro commented Sep 25, 2023

Hi @Delta456

I have not had a chance to look at this in great detail & verify it, however I have theorised what the issue may be.

This PR adds code to allow the map init checks to pass, however the array is still initialised with the wrong type.

What I am saying is that the array init [0xba, 0xaa] inside of

const m = {
	'foo':    [u8(0xf0), 0x00]
	'bar':    [0xba, 0xaa]
	'foobar': [12, 45]
}

is still initialised as it normally would be with the type of the first element int and not u8. I believe that due to this the array init code is generated incorrectly ([]int instead of []u8), or possibly some other initialisation code is wrong (empty array init const?). I think this is causing the failure.

There are a couple of ways this could be fixed. I will outline them.

A) you could add some type of state field to checker which would tell the array init to use the expected type. For example: from inside map init you set c.expected_type to []u8 and add a field to checker c.allow_expected_type_coercion. Then Inside of array init you would check to see if this field is true, if so it would initialise the array using p.expected_type. This is just an example

B) You could directly modify the array init expression node, and add in a cast to u8 node for the first element, then the array init should behave accordingly. I'm not sure if this would work, or if it is even a good idea.

In conclusion, if this is the issue (it may not be), I'm not sure if the user convenience vs checker complexity ratio is justifiable. It just feels like there are a lot of checks in checker, and the more of this type of thing is added, the trickier it will become to modify checker in the future. That being said, I'm sure it could be implemented in a nice way.

What do you guy's think @medvednikov & @spytheman ?

@spytheman
Copy link
Member

I agree with you @joe-conigliaro . This feature is too complex to implement consistently in the current compiler, compared to the provided convenience.

@spytheman spytheman closed this Sep 26, 2023
@Delta456 Delta456 deleted the map_array_cast_first_type branch September 26, 2023 05:41
@Delta456
Copy link
Member Author

Delta456 commented Sep 27, 2023

I just hope there is a much better and cleaner approach to implement this in the new checker and parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arrays in maps should be inferred based on the first value in the map
3 participants