Skip to content

fix(auto): don't treat strings as owned on Julia 1.12 - #80

Closed
MilesCranmerBot wants to merge 3 commits into
MilesCranmer:mainfrom
MilesCranmerBot:pr/string-tracking
Closed

MilesCranmerBot wants to merge 3 commits into
MilesCranmer:mainfrom
MilesCranmerBot:pr/string-tracking

Conversation

@MilesCranmerBot

Copy link
Copy Markdown
Contributor

Summary

ismutabletype(String) is true on Julia 1.12 (memory-based layout), so the owned-type tracker's generic mutable-type rule classified strings as owned. Semantically strings have value semantics with no user-facing in-place mutation API, and the package's own model treats immutable values as safe to pass around.

This caused opaque/unresolved calls to spuriously report consuming string arguments (e.g. any @safe wrapper forwarding a String through a package function whose summary cannot be resolved).

AbstractString is now classified as neither tracked nor owned.

Test plan

  • New regression testsets: string forwarding through an opaque callee, and a string stashed into a Dict then reused (both previously violated, now clean).
  • BORROWCHECKER_ONLY_AUTO=1 julia --project=. -e 'using Pkg; Pkg.test()': green; the two remaining @test_broken cases are fixed by fix(auto): stop false-positive generators in effect summarization #79 and intentionally still marked broken here.

@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: 3a56b60add

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/auto/ir_primitives.jl Outdated
@MilesCranmerBot

Copy link
Copy Markdown
Contributor Author

Addressed in f012a71: the exemption is now (T === String || T <: SubString) instead of T <: AbstractString, with a comment noting that user-defined mutable string subtypes must stay tracked. Verified that a hypothetical mutable struct MutStr <: AbstractString remains tracked+owned while String/SubString stay exempt.

The String/SubString ownership exemption and both testsets are merged
into MilesCranmer#88 (ported to the dissolved src/safe layout, green on 1.12 and
1.13-rc3).
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.

2 participants