8000 remove usage of method as function by Guest0x0 · Pull Request #2151 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

remove usage of method as function #2151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

remove usage of method as function #2151

wants to merge 1 commit into from

Conversation

Guest0x0
Copy link
Contributor

Previously, methods defined as fn f(self : T, ..) are considered both method and regular function. While this allows convenient ways of calling method via @pkg.meth(..), it also complicates the semantic of MoonBit.

The latest release of MoonBit now supports partial dot application (_.meth(..)) and piping into dot application (expr |> _.meth(..)), which significantly reduce the need for calling methods as regular functions. So we decided to deprecate the behavior of calling methods as regular functions, to simplify the language.

This PR removes usage of calling method as regular function in core. In next week's release, we will start emitting warnings on such usage. We are also simplifying the syntax of MoonBit's method system, the simplified syntax will also be available next week.

For most cases, this PR just migrates regular function apply to dot apply. However, for math related methods (Double::sin, for example), people are more used to function call style sin(..) from math notation, compared of dot apply style xx.sin(). So an fnalias is provided for math related functions and several others, such as Ref::swap.

@Guest0x0 Guest0x0 requested a review from bobzhang May 22, 2025 10:12
@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Math-related method usage inconsistency

Category
Maintainability
Code Snippet
double.mbt:
pub fn cbrt(self : Double) -> Double
pub fnalias Double::(sin, cos, tan, ...)
Recommendation
Consider adding a comment in the module documentation explaining why certain math functions have both method and function alias forms, while others don't. This will help users understand the design choices.
Reasoning
While the PR adds function aliases for math-related methods like sin/cos/tan, some methods like cbrt are still only available as methods. A clear documentation explaining this design decision would improve maintainability and user understanding.

Complex conversion logic could be simplified

Category
Maintainability
Code Snippet
string/deprecated.mbt:
pub fn String::op_as_view(self : String, start~ : Int = 0, end? : Int) -> View {
let str_len = self.length()
let start = if start >= 0 {
self.index_at_(start, start=0).unwrap()._
} else {
self.index_at_rev_(-start, end=str_len).unwrap()._
}
Recommendation
Consider extracting the index calculation logic into a separate helper function to improve readability and reusability
Reasoning
The string view conversion code contains duplicated logic for handling positive and negative indices. Extracting this into a helper function would reduce duplication and make the code more maintainable.

Potential missing function aliases

Category
Correctness
Code Snippet
double/math_functions.mbt:
pub fnalias Double::(sin, cos, tan, ...)
Recommendation
Review the list of math function aliases to ensure all commonly used mathematical functions are included. Consider adding aliases for less common but still useful functions.
Reasoning
The PR introduces function aliases for common math functions, but some functions might be missing. A comprehensive review would ensure consistency and completeness of the math function API.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6867

Details

  • 54 of 56 (96.43%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.451%
Changes Missing Coverage Covered Lines Changed/Added Lines %
immut/list/list.mbt 15 17 88.24%
Totals Coverage Status
Change from base Build 6866: 0.0%
Covered Lines: 8695
Relevant Lines: 9405
💛 - Coveralls

@bobzhang bobzhang enabled auto-merge (rebase) May 22, 2025 14:00
FlyCloudC
FlyCloudC approved these changes May 22, 2025
View reviewed changes
unit/README.mbt.md
@@ -23,7 +23,7 @@ Unit values can be converted to strings using either the standalone function or
test "unit string conversion" {
let u = ()
// Both ways produce the same result
inspect!(@unit.to_string(u), content="()")
inspect(u.to_string(), content="()")
Copy link
Contributor

Choose a reason for hiding this comment

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

missing !?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I just saw the latest commit that removed the !.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobzhang bobzhang bobzhang approved these changes

@FlyCloudC FlyCloudC FlyCloudC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Guest0x0 @coveralls @bobzhang @FlyCloudC
0