Skip to content

parser: check invalid struct name in struct_init()#26093

Merged
spytheman merged 2 commits intovlang:masterfrom
kbkpbot:fix-invalid-struct-init
Dec 24, 2025
Merged

parser: check invalid struct name in struct_init()#26093
spytheman merged 2 commits intovlang:masterfrom
kbkpbot:fix-invalid-struct-init

Conversation

@kbkpbot
Copy link
Copy Markdown
Contributor

@kbkpbot kbkpbot commented Dec 23, 2025

Fix issue #26030

check invalid struct name in struct_init().

@kbkpbot kbkpbot changed the title Fix invalid struct init parser: check invalid struct name in struct_init() Dec 23, 2025
Copy link
Copy Markdown
Contributor

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Excellent work.

Did you find out the root reason, that the previous message (from the issue) used the wrong .v file?
Can it happen in other cases too, that are not reported yet?
(it took a lot of time and dialogs on Discord to diagnose and isolate it)

@kbkpbot
Copy link
Copy Markdown
Contributor Author

kbkpbot commented Dec 23, 2025

Excellent work.

Did you find out the root reason, that the previous message (from the issue) used the wrong .v file? Can it happen in other cases too, that are not reported yet? (it took a lot of time and dialogs on Discord to diagnose and isolate it)

  1. While parsing f.v, the parser encounters the following code:
_ := http.Response{header: http.new_header{} }

Here, a name token followed by a { leads the parser to conclude that it must be an array_init or a struct_init.

v/vlib/v/parser/parser.v

Lines 1789 to 1801 in 0aa3e2d

} else if !known_var && (p.peek_tok.kind == .lcbr || is_generic_struct_init)
&& (!p.inside_match || (p.inside_select && prev_tok_kind == .arrow && lit0_is_capital))
&& !p.inside_match_case && (!p.inside_if || p.inside_select)
&& (!p.inside_for || p.inside_select) {
alias_array_type := p.alias_array_type()
if alias_array_type != ast.void_type {
return p.array_init(is_option, alias_array_type)
} else {
// `if a == Foo{} {...}` or `match foo { Foo{} {...} }`
return p.struct_init(p.mod + '.' + p.tok.lit, .normal, is_option)
}
} else if p.peek_tok.kind == .lcbr
&& ((p.inside_if && lit0_is_capital && p.tok.lit.len > 1 && !known_var && language == .v)

  1. The parser then determines it must be a struct_init and calls the p.struct_init() function.

  2. Within p.struct_init(), it uses p.parse_type() to parse the struct type—which in this case is the token new_header. As a result, net.http.new_header is incorrectly registered as a placeholder type.

v/vlib/v/parser/struct.v

Lines 552 to 556 in 0aa3e2d

fn (mut p Parser) struct_init(typ_str string, kind ast.StructInitKind, is_option bool) ast.StructInit {
first_pos := (if kind == .short_syntax && p.prev_tok.kind == .lcbr { p.prev_tok } else { p.tok }).pos()
p.init_generic_types = []ast.Type{}
mut typ := if kind == .short_syntax { ast.void_type } else { p.parse_type() }
struct_init_generic_types := p.init_generic_types.clone()

  1. Later, when parsing the file vlib/net/http/header.v, the parser encounters net.http.new_header again in the statement:
mut h := new_header()

It now assumes new_header refers to a type and interprets this as a cast() statement, expecting an expression inside the parentheses.

v/vlib/v/parser/parser.v

Lines 1715 to 1727 in 0aa3e2d

// type cast. TODO: finish
// if name in ast.builtin_type_names_to_idx {
// handle the easy cases first, then check for an already known V typename, not shadowed by a local variable
if (is_option || p.peek_tok.kind in [.lsbr, .lt, .lpar]) && (is_mod_cast
|| is_c_pointer_cast || is_c_type_cast || is_js_cast || is_generic_cast
|| (language == .v && name != '' && (is_capital_after_last_dot
|| name[0].is_capital()
|| (!known_var && (name in p.table.type_idxs || name_w_mod in p.table.type_idxs))))) {
// MainLetter(x) is *always* a cast, as long as it is not `C.`
// TODO: handle C.stat()
start_pos := p.tok.pos()
mut to_typ := p.parse_type()
// this prevents inner casts to also have an `&`

The root cause of this bug is that the parser incorrectly registers new_header as a type.

@kbkpbot
Copy link
Copy Markdown
Contributor Author

kbkpbot commented Dec 23, 2025

I'm not satisfied with this patch because it introduces excessive checks within the parser. I have an idea: could we skip these checks in the parser? For example, when checking an expression, instead of throwing an error, we could return an empty_expr. Then, during the subsequent real checks in the checker, the error would be caught.
Issue some error message like this:

/media/HD/github/kbkpbot/v/vlib/net/http/header.v:376:22: error: invalid expression: unexpected token `)`
  374 | // new_header_from_map creates a Header from key value pairs
  375 | pub fn new_header_from_map(kvs map[CommonHeader]string) Header {
  376 |     mut h := new_header()
      |                         ^
  377 |     h.add_map(kvs)
  378 |     return h

f.v:4:33: And the type `new_header` is registered here :
    2 |
    3 | import net.http
    4 | _ := http.Response{header: http.new_header{} }
      |                                 ~~~~~~~~~~
    5 |

@spytheman
Copy link
Copy Markdown
Contributor

That is one option indeed, that could make such messages a lot less confusing.

Another (perhaps complementary, and not alternative) is to somehow prevent the registration of types for other modules in the parser, based on incomplete code, and leaving the type checking and matching to the checker stage, where the type information is more complete (and can be analyzed in topological order, so that net.http is checked first, and thus its API is completely known, and only then the .v files in the main module.

@spytheman
Copy link
Copy Markdown
Contributor

Running ./v tracev and then ./tracev x.v for the code in the original issue, shows the files are parsed in this order, which seems wrong to me:

vlib/builtin/allocation.c.v
vlib/builtin/array.v
vlib/builtin/array_d_gcboehm_opt.v
vlib/builtin/backtraces.c.v
vlib/builtin/backtraces_nix.c.v
vlib/builtin/builtin.c.v
vlib/builtin/builtin.v
vlib/builtin/builtin_backtraces_nix.c.v
vlib/builtin/builtin_d_gcboehm.c.v
vlib/builtin/builtin_nix.c.v
vlib/builtin/builtin_notd_use_libbacktrace.c.v
vlib/builtin/cfns.c.v
vlib/builtin/cfns_wrapper.c.v
vlib/builtin/chan_option_result.v
vlib/builtin/character_inout.c.v
vlib/builtin/float.c.v
vlib/builtin/input_rune_iterator.v
vlib/builtin/int.v
vlib/builtin/map.c.v
vlib/builtin/map.v
vlib/builtin/map_d_gcboehm_opt.v
vlib/builtin/meta_assert.v
vlib/builtin/meta_attribute.v
vlib/builtin/meta_enum.v
vlib/builtin/meta_function.v
vlib/builtin/meta_struct.v
vlib/builtin/meta_variant.v
vlib/builtin/option.c.v
vlib/builtin/panicing.c.v
vlib/builtin/printing.c.v
vlib/builtin/reuse.v
vlib/builtin/rune.v
vlib/builtin/rune_map.v
vlib/builtin/sorted_map.v
vlib/builtin/string.v
vlib/builtin/string_charptr_byteptr_helpers.v
vlib/builtin/string_interpolation.v
vlib/builtin/utf8.c.v
vlib/builtin/utf8.v
/home/delian/code/misc/2025_12_23__16/x.v
vlib/strings/builder.c.v
vlib/strings/similarity.v
vlib/strings/strings.c.v
vlib/strings/strings.v
vlib/builtin/closure/closure.c.v
vlib/builtin/closure/closure_nix.c.v
vlib/strconv/atof.c.v
vlib/strconv/atofq.c.v
vlib/strconv/atoi.v
vlib/strconv/atou.v
vlib/strconv/f32_str.c.v
vlib/strconv/f64_str.c.v
vlib/strconv/f64_str.v
vlib/strconv/format.v
vlib/strconv/format_mem.c.v
vlib/strconv/ftoa.c.v
vlib/strconv/number_to_base.c.v
vlib/strconv/structs.v
vlib/strconv/tables.v
vlib/strconv/utilities.c.v
vlib/strconv/utilities.v
vlib/strconv/vprintf.c.v
vlib/net/http/backend.c.v
vlib/net/http/cookie.v
vlib/net/http/download.v
vlib/net/http/download_progress.v
vlib/net/http/download_silent_downloader.v
vlib/net/http/download_terminal_downloader.v
vlib/net/http/header.v

Ideally this should not matter, if the parser does not try to do type checking, and just produces a concrete AST from each file (which could have other benefits like future parallelization) , but unfortunately currently the parser does use its ast.table for checks, and not just registrations, so it is sensitive to ordering issues :-| .

@JalonSolov
Copy link
Copy Markdown
Collaborator

Making the parser only parse, and moving the checking for correctness to the checker, makes logical sense.

V has too much "bleed-through" between stages... parser does checks, cgen creates methods, etc.

Cleaning all that up so that the various stages only do what they are supposed to do should make V much easier to maintain.

@kbkpbot
Copy link
Copy Markdown
Contributor Author

kbkpbot commented Dec 24, 2025

That is one option indeed, that could make such messages a lot less confusing.

Another (perhaps complementary, and not alternative) is to somehow prevent the registration of types for other modules in the parser, based on incomplete code, and leaving the type checking and matching to the checker stage, where the type information is more complete (and can be analyzed in topological order, so that net.http is checked first, and thus its API is completely known, and only then the .v files in the main module.

Yes, I think the parser should not register type.
In developing PR #25365, I found it is very hard to change the types after parser.

@spytheman
Copy link
Copy Markdown
Contributor

I am merging the PR as it is for now, since it does prevent a weirder situation, with a minimal change.

I am also glad of the discussion that followed, and that we all understand what the ideal for the parser should be, and why it would be useful to move in that direction. Thank you 🙇🏻‍♂️ .

@spytheman spytheman merged commit d1ed866 into vlang:master Dec 24, 2025
85 of 87 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