Skip to content
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.

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

fix: method naming conflicts #215

Closed

Conversation

osalkanovic
Copy link
Contributor

Issue: #121

Here is working example how to prevent same functions naming as c functions.
I tried to move functions to helper, but unsuccessfully to import helper inside babel transformer, if someone can assist it would be good :))

@@ -25,6 +28,7 @@ export default function () {
}
}
for (let method of Object.keys(contractMethods)) {
validateMethod(method, cFunctions)
Copy link
Member

Choose a reason for hiding this comment

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

This validating pattern is cool!


function getCFunctions(){
const builderRaw = readFileSync(path.resolve('../cli/builder/builder.c'), 'utf-8')
const regex = new RegExp(/(?<=JS_SetPropertyStr\(ctx, env, ").+?(?=", JS_NewCFunction\(ctx,)/gm)
Copy link
Member

@ailisp ailisp Sep 13, 2022

Choose a reason for hiding this comment

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

There are more functions to filter out, basically every C function that's included in builder.c, such as strncpy, so this is quite challenging to figure out all C function names. I wonder whether one these directions can be easier:

  • when confliction happens, can we collect the C compiler's error/warning and indicate the user to rename the contract method
  • can we iterate contract method names like you did, then for any name confliction, use some C code-transformation / compiler tool to rename C functions to some random names. For example, a developer can write a contract method named strncpy (:joy:), and we let the tool to rename original C strncpy (not user contract method name) to ___strncpy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wooow, yes. Why not, I will update PR

@volovyks volovyks linked an issue Sep 13, 2022 that may be closed by this pull request
@volovyks
Copy link
Collaborator

The reason why you can not import these functions from utils is that Babel not allowing imports without .js file extension.
https://stackoverflow.com/questions/67996772/babel-not-allowing-imports-without-js-file-extension

You are importing it from ../utils, but Babel expects ../unils.js from /lib folder. You can try to apply the suggested solution, but it is not a big deal.

@ailisp
Copy link
Member

ailisp commented Sep 15, 2022

@osalkanovic Looks like a yarn build to update the built artifacts and commit again is required to pass CI

@osalkanovic
Copy link
Contributor Author

@ailisp can you re-run CI?

@osalkanovic
Copy link
Contributor Author

osalkanovic commented Sep 16, 2022

@ailisp first step is resolved
output from groupCFunctions is https://gist.github.com/osalkanovic/062d308544159747950a6e4abf074c02

Now we have coordinates where function start in builder.c so we can easily include __ in function name :D

I have to also found coordinates for regex match functions :D
I will update PR with step 2 in next days :)

cc @volovyks can you assist with moving functions to utils?

@osalkanovic
Copy link
Contributor Author

CI should fail because I have c function in smart contract

@osalkanovic osalkanovic requested review from ailisp and removed request for volovyks September 16, 2022 16:53
@ailisp
Copy link
Member

ailisp commented Sep 19, 2022

@osalkanovic Seems that your collectCFunction/groupCFunction implemented a great algorithm that parsed all c function used in builder.c! However it doesn't collect the names in #include files. The C programming language is quite low level, there's no namespace or module concept, the #Include is equivalent to litterally copy-paste such file in place. So the function defines in #include (unless static ones, which are file-local names), will also conflict.

I suggest to look for some existing unix tool that recursively process C includes, for example ctags might work. The good news about the include files are they are almost fixed and we almost never change them, so a one time figure out the reserved function names from include files is good enough. For builder.c it can be changed in future near-sdk-js and your automatic groupCFunction is nice!

@osalkanovic osalkanovic marked this pull request as draft September 20, 2022 10:44
@volovyks
Copy link
Collaborator

@ailisp first step is resolved output from groupCFunctions is https://gist.github.com/osalkanovic/062d308544159747950a6e4abf074c02

Now we have coordinates where function start in builder.c so we can easily include __ in function name :D

I have to also found coordinates for regex match functions :D I will update PR with step 2 in next days :)

cc @volovyks can you assist with moving functions to utils?

We can move functions later. That is a separate issue.

@osalkanovic Can we continue with this PR? We are happy to help you if you are stuck with something.
It's an important feature and the code is getting outdated...

@ailisp
Copy link
Member

ailisp commented Feb 24, 2023

Fixed in #346 . @osalkanovic Appreciate your contributions to near-sdk-js! We are now accepting proposals and give fundings to improve NEAR developer tools, including JS SDK. If you have an idea about near-sdk-js, create an Idea post labeled with near-sdk-js and reply to existing ideas with Solution post with your proposal and financial ask. We'll discuss them on a regular basis and fund proposals that pass the vote in our work group. Then you can start work on it with funding.

The place to submit idea / solution: https://devgovgigs.near.social

@ailisp ailisp closed this Feb 24, 2023
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.

Standalone contract function name conflicting issue
3 participants