Skip to content

checker: error on empty integer range in for-in loop#27833

Merged
JalonSolov merged 5 commits into
vlang:masterfrom
arshalaromal:empty-range-checker
Jul 20, 2026
Merged

checker: error on empty integer range in for-in loop#27833
JalonSolov merged 5 commits into
vlang:masterfrom
arshalaromal:empty-range-checker

Conversation

@arshalaromal

Copy link
Copy Markdown
Contributor

This PR adds a semantic check to prevent empty hardcoded integer ranges (e.g., for i in 4 .. 2) in for ... in loops.

Because V ranges are exclusive (start .. end), a loop where start >= end will never execute. Previously, this would silently compile and skip the loop entirely. This change aligns with V's strictness by catching this unreachable code and potential typo at compile time with a hard error.

Resolves #27720

Tests

Added for_in_empty_range_err.vv and for_in_empty_range_err.out to vlib/v/checker/tests/ to cover this specific compiler error.

@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 665218db47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/checker/for.v Outdated
@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a971c98cdb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/checker/tests/for_in_empty_range_err.out Outdated
@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 28adf68b1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Ok for me its good now.

@arshalaromal

Copy link
Copy Markdown
Contributor Author

I've updated the test expectation to 2:14 as requested

@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Sorry, I gave you the wrong instruction. I ran the targeted test before running v fmt on the .vv file. v fmt changes the indentation to tabs, making 2:11 the correct position. I retested it and the test passes with 2:11. Please just drop your latest commit and make sure to run v fmt on vlib/v/checker/tests/for_in_empty_range_err.vv.

Thanks!

@arshalaromal
arshalaromal force-pushed the empty-range-checker branch from a37a574 to 28adf68 Compare July 18, 2026 15:41
@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Your latest commit correctly restored 2:11, but it accidentally added cmd/tools/vbug-report-send.lock and the relevant files are still not formatted. Please remove the lock file and run v fmt on vlib/v/checker/for.v and vlib/v/checker/tests/for_in_empty_range_err.vv.

Thanks :)

@arshalaromal
arshalaromal force-pushed the empty-range-checker branch from 86e9b3d to 28adf68 Compare July 18, 2026 15:46
@arshalaromal

Copy link
Copy Markdown
Contributor Author

i am sorry, is it okay now?

@arshalaromal
arshalaromal force-pushed the empty-range-checker branch from 28adf68 to 0815ec5 Compare July 18, 2026 15:49
@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

On the current HEAD, these two files are still not vfmt'ed for me:

vlib/v/checker/for.v
vlib/v/checker/tests/for_in_empty_range_err.vv

@arshalaromal

Copy link
Copy Markdown
Contributor Author

Ah, my bad..
Fixed the formatting.
Thanks for the patience and for pointing it out.

@GGRei

GGRei commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

@JalonSolov

Copy link
Copy Markdown
Collaborator

If you run v git-fmt-hook install, it will install a pre-commit hook that will format your files before they are committed, so when they are pushed you don't have to worry about it.

@medvednikov medvednikov reopened this Jul 18, 2026
@arshalaromal

Copy link
Copy Markdown
Contributor Author

I noticed the CI failed on generic_fn_short_syntax_struct_param_test.v. I reproduced the error locally, but then I switched to the master branch and ran the test there, and it fails with the exact same error (unknown struct T). It looks like this is an existing issue on master and unrelated to my range checker changes.

@arshalaromal
arshalaromal requested a review from GGRei July 20, 2026 03:32
@GGRei

GGRei commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Yes, at first glance, the CI errors didn’t seem to come from your PR. I can see that Alex patched all of that recently as well. You did the right thing by rebasing onto the upstream branch to check the CI again. In any case, I can’t merge your work myself here, @medvednikov is the one who gives the final approval. :)

@JalonSolov

Copy link
Copy Markdown
Collaborator

Everything looks good for this. The CI failures do appear to be unrelated.

@JalonSolov
JalonSolov merged commit 13da313 into vlang:master Jul 20, 2026
89 of 96 checks passed
@arshalaromal
arshalaromal deleted the empty-range-checker branch July 20, 2026 11:51
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.

add a warning/error when a for-in loop range is empty

4 participants