Skip to content

Conversation

Thesola10
Copy link
Contributor

Fixes #673.

Proof-of-concept, feedback is welcome!

@Thesola10
Copy link
Contributor Author

Thesola10 commented Feb 10, 2025

From a review

the v0 shouldn't be hardcoded, as there may be undefined number of variations of the same function. not sure how you should select a variation in this situation, though

... I guess we forbid nameof on overloads. nameof implies breaking out of the Amber type system so there's no way to clearly select a variant anyway.

The alternative is compiling variants in a "turntable" function which would be returned by nameof, and handle type-checking the number and type of arguments at runtime. The turntable would only be compiled for functions nameof was called on.

Or a special syntax to denote the expected argument set, like nameof my_fun(Text, [Number]) in these situations.

@Mte90 Mte90 requested review from hdwalters and Ph0enixKM February 18, 2025 10:01
@hdwalters hdwalters changed the base branch from main to staging February 19, 2025 21:20
@hdwalters
Copy link
Contributor

I've also taken the liberty of changing the merge base from main to staging. I don't think this will cause any problems.

@Thesola10
Copy link
Contributor Author

Ah found an issue: this nameof doesn't mark the function as used, so if it's the only instance the function just doesn't get compiled.

@Thesola10 Thesola10 requested a review from hdwalters February 24, 2025 10:54
Thesola10 added a commit to nixie-dev/nixie that referenced this pull request Feb 26, 2025
Rewrote the bash wrapper script into Amber

This temporarily introduces a hard dependency on bc and sed,
at least until amber-lang/amber#366 is fixed, and the script is
built with amber-lang/amber#674 applied to the compiler.
use crate::modules::variable::variable_name_extensions;
use crate::translate::module::TranslateModule;
use crate::utils::{ParserMetadata, TranslateMetadata};
use crate::modules::function::invocation_utils::handle_function_parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sort use statements alphabetically, to simplify future maintenance.

self.name = format!("{}__{}_v0", fun_decl.name, fun_decl.id);

// Create an empty call to ensure referenced function gets built
let fun_decl2 = fun_decl.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're not using fun_decl after this, it would be cleaner to shadow the variable name:

let fun_decl = fun_decl.clone();

@hdwalters
Copy link
Contributor

I'm still not entirely happy with the hard-coded "v0" in your code change. However, I suggest that we defer further review on this PR until @Ph0enixKM's PR is merged, because AIUI it touches Bash variable name generation, and will consequently have an impact on this PR.

};
Ok(())
} else if let Some(fun_decl) = meta.get_fun_declaration(&name) {
self.name = format!("{}__{}_v0", fun_decl.name, fun_decl.id);
Copy link
Member

@b1ek b1ek Mar 10, 2025

Choose a reason for hiding this comment

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

@Ph0enixKM is there even a way to properly select a variation of a function? i honestly have no idea how to go around this. hardcoding is definitely not the way for obvious reasons

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Now since we're on a different architecture, we can slowly start to build centralized system that names these functions a certain way and only ask the system for the appropriate name here

@Mte90
Copy link
Member

Mte90 commented Apr 10, 2025

@Thesola10 can you update with the feedback and the latest changes?

@Mte90 Mte90 requested a review from hdwalters April 16, 2025 10:44
@b1ek b1ek marked this pull request as draft May 10, 2025 10:25
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.

[BUG] nameof does not work on functions
5 participants