Skip to content

v.ast: add Table.update_sym_by_idx/2#25373

Merged
spytheman merged 4 commits into
vlang:masterfrom
kbkpbot:fix-rewrite_already_registered_symbol
Sep 24, 2025
Merged

v.ast: add Table.update_sym_by_idx/2#25373
spytheman merged 4 commits into
vlang:masterfrom
kbkpbot:fix-rewrite_already_registered_symbol

Conversation

@kbkpbot

@kbkpbot kbkpbot commented Sep 22, 2025

Copy link
Copy Markdown
Contributor

Allow update existing symbol via call update_sym().

a use case:
To support the new array syntax, we need to update the fixed-arrays size after scan the array lit. For example,

[..][..]int[[1 2][3 4]]

When parse this, we don't known the size of the fixed-array first, but register a [-1][-1]int symbol in the system.
And then after scan the array lit, [[1 2][3 4]], we can update the existing [-1][-1]int symbol to [2][2]int.

Comment thread vlib/v/ast/types.v Outdated

@spytheman spytheman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new method does not use the whole map, it just needs its length.

@spytheman

Copy link
Copy Markdown
Contributor

Can you please also add a test case?

@vlang vlang deleted a comment from huly-for-github Bot Sep 22, 2025
@kbkpbot kbkpbot changed the title ast: force cache update when rewrite_already_registered_symbol ast: add update_sym() Sep 23, 2025
Comment thread vlib/v/ast/table.v Outdated
@spytheman spytheman changed the title ast: add update_sym() v.ast: add Table.update_sym_by_idx/2 Sep 24, 2025
Comment thread vlib/v/ast/types.v
// delete_cached_type_to_str remove a `type_to_str` from the cache
pub fn (t &Table) delete_cached_type_to_str(typ Type, import_aliases_len int) {
cache_key := (u64(import_aliases_len) << 32) | u64(typ)
mut mt := unsafe { &Table(t) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just mut t Table for the receiver?

It is only called by update_sym_by_idx, which already has a mutable receiver 🤔 .

@spytheman spytheman Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this new method, will need to loop over all cached_type_to_str entries and delete the ones that have typ in the lower bits of the key.

Otherwise the strings for the same type, could be cached in different entries, only differing by their high parts of the key (which is depending on the declared aliases in each .v file).

Previously that was fine, since the type symbols were immutable, but if we allow updates, that cache can go stale, and lead to bugs, and we need to be careful to update or invalidate it properly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. the new signature should be something like this:

pub fn (mut t Table) delete_cached_type_to_str(typ Type) {

@spytheman

Copy link
Copy Markdown
Contributor

Thinking about it more, would not it be easier, to just register a placeholder symbol, while parsing the [..]int[10,20], then after the parser knows the complete information about the size, register a completely new type and symbol, based on the complete information, without updating the old placeholder one?

Then we would not have to invalidate any caches, and cgen could just skip the placeholder symbols completely.

@kbkpbot

kbkpbot commented Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Thinking about it more, would not it be easier, to just register a placeholder symbol, while parsing the [..]int[10,20], then after the parser knows the complete information about the size, register a completely new type and symbol, based on the complete information, without updating the old placeholder one?

Then we would not have to invalidate any caches, and cgen could just skip the placeholder symbols completely.

Think about it, dealing with array declarations like [5][..][][const][..]int feels quite tricky.

@spytheman

Copy link
Copy Markdown
Contributor

Think about it, dealing with array declarations like [5][..][][const][..]int feels quite tricky.

You may be right, but I do not understand why?

In one case, you will have to update an existing symbol, and deal with the cache invalidations/updates (because currently the assumption is that the type/symbol information does not change much once registered).

In the other, you will be copying an existing placeholder symbol on each recursion level, changing it in the process, and storing a new version of it (under a new type id), leaving the old one as it is - the cached entries in the table will continue to work for the placeholder types/symbols, and after the parsing is done, you would have a valid and complete type for the full array init. The negative could be using more memory for the new symbols, and potentially some logic in cgen to skip the incomplete ones.

@spytheman

Copy link
Copy Markdown
Contributor

Anyway, given that you have a clear picture of how to do it with your approach, while I have not considered all implications of mine, I do not want to block it - I will merge this PR as it is, and we will see how it will pan out. If there are too many bugs found due to the cache maintenance, we could then reconsider etc.

@spytheman spytheman merged commit 5bc82ed into vlang:master Sep 24, 2025
82 checks passed
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.

3 participants