Skip to content

Use DeclEngine::insert_modified instead of replace - #7717

Open
ironcev wants to merge 1 commit into
masterfrom
ironcev/use-insert-modified-instead-of-replace
Open

Use DeclEngine::insert_modified instead of replace#7717
ironcev wants to merge 1 commit into
masterfrom
ironcev/use-insert-modified-instead-of-replace

Conversation

@ironcev

@ironcev ironcev commented Aug 14, 2026

Copy link
Copy Markdown
Member

Description

This PR is a prerequisite for removals of duplicates from DeclEngine and for elimination of expensive hashing.

The PR removes most of the usages of DeclEngine::replace, up to those used in type_check_impl_self and in type_check_method_application (the usage in impl ReplaceFunctionImplementingType for DeclRefFunction we consider to be a part of type_check_impl_self). Those remaining usages will be removed in a separate follow up PR.

Beside being a show-stopper for DeclEngine improvements, DeclEngine::replace usage in the cases tackled in this PR was error-prone. Its callers assumed that the replaced TyDecl was not reused by different declarations. As the all passing tests confirm, this assumption currently holds. But note that we already had issues in the past with TyConstantDecl where replacing shared declarations was causing side-effects and causing bugs in the compilation. impl SubstTypes for DeclId<TyConstantDecl> was already using insert_modified instead of replace with a comment explaining why. This PR merely roles out that correct pattern to all TyDecls.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@ironcev
ironcev deployed to fuel-sway-bot August 14, 2026 13:18 — with GitHub Actions Active
@cursor

cursor Bot commented Aug 14, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches core compiler declaration identity during substitution and trait method resolution; behavior change is intentional (copy-on-write) but wrong wiring could break monomorphization or shared decl references.

Overview
Replaces in-place DeclEngine::replace with insert_modified when type substitution or decl replacement produces a modified typed declaration, so callers get a new DeclId/DeclRef instead of overwriting a slot that might still be shared (e.g. monomorphized functions or constants).

SubstTypes for all DeclId<…> declaration kinds now assigns *self = *decl_engine.insert_modified(decl, *self).id() after a successful subst. The generic SubstTypes impl on DeclRef<DeclId<T>> does the same via *self = decl_engine.insert_modified(...), with added DeclEngineInsert and TyDeclParsedType bounds. Function application replace_decls updates the method fn_ref the same way when trait constraint replacement changes the function decl.

The long comment on DeclId<TyConstantDecl> explaining why replace was wrong is removed because that behavior is now the default for every TyDecl kind in these paths. Remaining replace usages (e.g. type_check_impl_self, method application type-check, replace_implementing_type) are intentionally left for a follow-up.

Reviewed by Cursor Bugbot for commit 01d1aff. Bugbot is set up for automated code reviews on this repo. Configure here.

@ironcev ironcev self-assigned this Aug 14, 2026
@ironcev ironcev added compiler General compiler. Should eventually become more specific as the issue is triaged compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Aug 14, 2026
@ironcev

ironcev commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

👍

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 01d1aff. Configure here.

Comment thread sway-core/src/language/ty/expression/expression_variant.rs
Comment thread sway-core/src/language/ty/expression/expression_variant.rs
@codspeed-hq

codspeed-hq Bot commented Aug 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 25 untouched benchmarks


Comparing ironcev/use-insert-modified-instead-of-replace (01d1aff) with master (8e74e26)

Open in CodSpeed

@ironcev
ironcev marked this pull request as ready for review August 14, 2026 13:47
@ironcev
ironcev requested a review from a team as a code owner August 14, 2026 13:47
@ironcev
ironcev enabled auto-merge (squash) August 14, 2026 13:47
@ironcev
ironcev requested a review from xunilrj August 14, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler General compiler. Should eventually become more specific as the issue is triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant