Skip to content

Make comptime functions ineligible for coverage - #162354

Open
Zalathar wants to merge 2 commits into
rust-lang:mainfrom
Zalathar:comptime
Open

Make comptime functions ineligible for coverage#162354
Zalathar wants to merge 2 commits into
rust-lang:mainfrom
Zalathar:comptime

Conversation

@Zalathar

@Zalathar Zalathar commented Sep 6, 2026

Copy link
Copy Markdown
Member

As reported in #161808, the assertion in #159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument core, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.


This PR is essentially a rewrite of #161808. I wanted to request several modifications to that PR, and the overall change is small enough that it was easier for me to just recreate it from scratch.

The main differences are:

  • Adds a coverage test instead of a ui test
  • Comments focus more on the fact that instrumenting comptime functions isn't useful, with avoiding the ICE being a secondary motivation

cc @fmease @oli-obk as I'm not familiar with comptime

@Zalathar Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Sep 6, 2026
@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2026
@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

r? @mati865

rustbot has assigned @mati865.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 75 candidates
  • Random selection from 19 candidates

@fs-rachel

Copy link
Copy Markdown
Contributor

Looks reasonable to me, I'd like to ask for two things to be brought over from my version:

  1. Please can you add Co-Authored-By: Rachel Barker <rachel.barker@ferrous-systems.com> to the commit message?

  2. Please can you add the test first in its own commit, so that it's easy to check that the test fails before the main change is applied and succeeds afterward?

@rustbot

rustbot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Zalathar and others added 2 commits September 7, 2026 19:58
This test demonstrates the existing crash, and will be migrated to a successful
coverage test in a subsequent commit.

Co-Authored-By: Rachel Barker <rachel.barker@ferrous-systems.com>
Compile-time-only functions don't generate code, so instrumenting them for
coverage is useless.

This also avoids an ICE when trying to get the function's symbol name for an
unused-function record, which can occur when instrumenting `core`.

Co-Authored-By: Rachel Barker <rachel.barker@ferrous-systems.com>
@Zalathar

Zalathar commented Sep 7, 2026

Copy link
Copy Markdown
Member Author
  1. Please can you add Co-Authored-By: Rachel Barker <rachel.barker@ferrous-systems.com> to the commit message?

Sounds reasonable, done.

  1. Please can you add the test first in its own commit, so that it's easy to check that the test fails before the main change is applied and succeeds afterward?

Normally I wouldn't add a failing test in its own commit, specifically because I don't want to have a failing intermediate commit.

Since you asked, what I've done instead is temporarily add the test as a tests/crashes test that “successfully” crashes, and then migrate it over to tests/coverage in the commit that fixes the crash.

(If it were a tests/ui test, I could just add a test that expects to crash, and then change the test expectation from crash to pass. But coverage tests aren't allowed to crash the compiler.)

@fs-rachel

Copy link
Copy Markdown
Contributor
  1. Please can you add Co-Authored-By: Rachel Barker <rachel.barker@ferrous-systems.com> to the commit message?

Sounds reasonable, done.

Thanks :)

  1. Please can you add the test first in its own commit, so that it's easy to check that the test fails before the main change is applied and succeeds afterward?

Normally I wouldn't add a failing test in its own commit, specifically because I don't want to have a failing intermediate commit.

Since you asked, what I've done instead is temporarily add the test as a tests/crashes test that “successfully” crashes, and then migrate it over to tests/coverage in the commit that fixes the crash.

(If it were a tests/ui test, I could just add a test that expects to crash, and then change the test expectation from crash to pass. But coverage tests aren't allowed to crash the compiler.)

Fair enough, I'm happy with that

@oli-obk oli-obk 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.

I'm changing how comptime fns are handled to make all this a bit simpler, but for now, this is the correct check and mirrors the other checks in the compiler

r? @oli-obk

@bors r+ rollup

View changes since this review

@rust-bors

rust-bors Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 0027ee1 has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned mati865 Sep 7, 2026
@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage

- rust-lang#159777
- rust-lang#161808
---

As reported in rust-lang#161808, the assertion in rust-lang#159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument `core`, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage

- rust-lang#159777
- rust-lang#161808
---

As reported in rust-lang#161808, the assertion in rust-lang#159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument `core`, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.
rust-bors Bot pushed a commit that referenced this pull request Sep 7, 2026
…uwer

Rollup of 3 pull requests

Successful merges:

 - #161896 (Remove fields from TypeKind: Array, Slice)
 - #162354 (Make comptime functions ineligible for coverage)
 - #162379 (add test ensuring we refuse to const-eval the body of a rustc_do_not_const_check function)

Failed merges:

 - #162294 (Reflection refactor ptrs)
@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors try jobs=test-armhf-gnu

rust-bors Bot pushed a commit that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage


try-job: test-armhf-gnu
@rust-bors

This comment has been minimized.

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage

- rust-lang#159777
- rust-lang#161808
---

As reported in rust-lang#161808, the assertion in rust-lang#159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument `core`, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage

- rust-lang#159777
- rust-lang#161808
---

As reported in rust-lang#161808, the assertion in rust-lang#159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument `core`, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.
@rust-bors

rust-bors Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: fff678a (fff678a10a9c4a1f5f465322647dd2f3af976448)
Base parent: 656a9da (656a9da186dacaf3bf8f7f7296a825d256cb4ae3)

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Sep 7, 2026
Make comptime functions ineligible for coverage

- rust-lang#159777
- rust-lang#161808
---

As reported in rust-lang#161808, the assertion in rust-lang#159777 causes the compiler to ICE when it tries to generate an unused-function record for comptime functions, because comptime functions aren't allowed to have a symbol name. This can occur when trying to instrument `core`, for example.

Compile-time-only functions don't generate code, so instrumenting them for coverage is useless anyway. This PR therefore makes them ineligible for coverage, which avoids the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants