feat[lang]!: move sqrt to new stdlib `math` module by cyberthirst · Pull Request #4520 · vyperlang/vyper

cyberthirst · 2025-03-13T13:58:29Z

What I did

implements #4515

How I did it

How to verify it

Commit message

this commit moves the `sqrt()` builtin to a new, pure vyper stdlib
implementation.

it includes machinery to handle extension of the stdlib in the
future. the benefits of having machinery to handle code written in pure
vyper in the stdlib is that it will be easier for external contributors
to contribute and audit. it also uses the native import machinery,
rather than the existing ad-hoc mechanism for writing vyper from inside
the compiler, which simplifies the implementation. that mechanism for
writing pure vyper builtins is also removed in this commit (previously,
this machinery had been used specifically to handle "pure vyper"
builtins, the only one of which was `sqrt()`).

Description for the changelog

Cute Animal Picture

codecov · 2025-03-13T14:17:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.63%. Comparing base (9486a41) to head (74c40e1).
Report is 81 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4520      +/-   ##
==========================================
- Coverage   92.71%   92.63%   -0.09%     
==========================================
  Files         123      122       -1     
  Lines       17546    17518      -28     
  Branches     2977     2976       -1     
==========================================
- Hits        16268    16228      -40     
- Misses        879      892      +13     
+ Partials      399      398       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cyberthirst · 2025-03-13T15:29:00Z

addresses the sqrt part of #4520
after discussion with @charles-cooper isqrt won't be ported because it would introduce a gas regression as it is hand-rolled via IR

vyper/builtins/functions.py

    ZeroDivisionException,
 )
-from vyper.semantics.analysis.base import Modifiability, VarInfo
+from vyper.semantics.analysis.base import Modifiability


charles-cooper · 2025-03-18T16:22:29Z

vyper/semantics/analysis/imports.py

    remapped_module = module_str
-    if remapped_module.startswith("ethereum.ercs"):
+    if is_erc_interface:
        remapped_module = remapped_module.removeprefix("ethereum.ercs")


maybe we should physically move these to the stdlib/ directory

(to fuse the two branches)

won't it still require a condition?

updated the remapping logic a bit to use a dict: 828d3ea

charles-cooper · 2025-03-18T16:54:41Z

tests/functional/codegen/types/numbers/test_sqrt.py


 def test_sqrt_literal(get_contract):
    code = """
+import stdlib.math as math


i prefer just import math

hmm, I don't. I think that stdlib clearly establishes that it comes from the compiler. I think that it's reasonable to assume that math module could be from user-space

How about vyper.math

import math was added in b5e3825

cyberthirst · 2025-04-03T06:41:21Z

trying to close&reopen to reflect the newest changes, due to gh bug the new commits weren't reflected

charles-cooper · 2025-04-10T11:03:40Z

vyper/semantics/analysis/imports.py

+

 def _load_builtin_import(level: int, module_str: str) -> tuple[CompilerInput, vy_ast.Module]:
-    if not _is_builtin(module_str):  # pragma: nocover


i don't think the pragma should be removed

charles-cooper · 2025-04-10T11:05:33Z

vyper/semantics/analysis/imports.py

+    for prefix in BUILTIN_PREFIXES:
+        if module_str.startswith(prefix):
+            return prefix
+    return None


panic here, unreachable

Suggested change

return None

raise CompilerPanic("unreachable") # pragma: nocover

charles-cooper · 2025-04-10T11:07:06Z

vyper/semantics/analysis/imports.py

+
+
+def _get_builtin_prefix(module_str: str):
+    for prefix in BUILTIN_PREFIXES:


nit:

Suggested change

for prefix in BUILTIN_PREFIXES:

for prefix in BUILTIN_PREFIXES.keys():

charles-cooper

couple nits, but looks good overall

cyberthirst · 2025-04-12T10:05:32Z

looked at the recent commits by @charles-cooper, lgtm

cyberthirst added 4 commits March 13, 2025 14:27

remove sqrt from builtins

f89de33

remove sqrt from builtins

48eb934

update builtin imports

1cb114b

update sqrt tests

56e16c5

cyberthirst added 2 commits March 13, 2025 16:51

add some sqrt import tests

a531ee5

delete sqrt machinery

8c7e866

github-advanced-security bot found potential problems Mar 13, 2025

View reviewed changes

remove stray test

07b837d

cyberthirst added this to the v0.4.2 milestone Mar 13, 2025

cyberthirst marked this pull request as ready for review March 17, 2025 06:47

charles-cooper added the release - must release blocker label Mar 17, 2025

charles-cooper reviewed Mar 18, 2025

View reviewed changes

cyberthirst added 3 commits March 29, 2025 10:56

raise when old sqrt builtin is referenced

f1d8759

allow direct import of math module

b5e3825

update remmaping logic

828d3ea

cyberthirst closed this Apr 3, 2025

cyberthirst reopened this Apr 3, 2025

cyberthirst requested a review from charles-cooper April 4, 2025 12:24

cyberthirst changed the title ~~feat[lang]: move sqrt to stdlib~~ feat[lang]!: move sqrt to stdlib Apr 10, 2025

charles-cooper reviewed Apr 10, 2025

View reviewed changes

charles-cooper added 3 commits April 12, 2025 10:15

clean up duplicated code, flow of builtin detection

d873d57

fix _get_builtin_prefix

39b0f8c

update error for deprecated sqrt() builtin

5135239

charles-cooper approved these changes Apr 12, 2025

View reviewed changes

charles-cooper changed the title ~~feat[lang]!: move sqrt to stdlib~~ feat[lang]!: move sqrt to new stdlib math module Apr 12, 2025

Merge branch 'master' into feat/move-sqrt-to-stdlib

74c40e1

charles-cooper enabled auto-merge (squash) April 12, 2025 12:49

charles-cooper merged commit 1591a12 into vyperlang:master Apr 12, 2025
159 checks passed

pcaversaccio mentioned this pull request Apr 13, 2025

Do not ban relative imports of math #4576

Open

charles-cooper mentioned this pull request Apr 14, 2025

reimplement sqrt and isqrt in pure vyper and remove them from builtins #4515

Closed



		def _load_builtin_import(level: int, module_str: str) -> tuple[CompilerInput, vy_ast.Module]:
		if not _is_builtin(module_str): # pragma: nocover

	return None
	raise CompilerPanic("unreachable") # pragma: nocover



		def _get_builtin_prefix(module_str: str):
		for prefix in BUILTIN_PREFIXES:

	for prefix in BUILTIN_PREFIXES:
	for prefix in BUILTIN_PREFIXES.keys():

Uh oh!

Conversation

cyberthirst commented Mar 13, 2025 • edited by charles-cooper Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

Uh oh!

codecov bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cyberthirst commented Mar 13, 2025

Uh oh!

Check notice

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberthirst commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

cyberthirst commented Apr 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyberthirst commented Mar 13, 2025 •

edited by charles-cooper

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading