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

Repro and Fix for issue #2529 #2531

Merged
merged 7 commits into from
May 17, 2021
Merged

Repro and Fix for issue #2529 #2531

merged 7 commits into from
May 17, 2021

Conversation

crusso
Copy link
Contributor

@crusso crusso commented May 15, 2021

No description provided.

@crusso crusso changed the title Repro for issue 2529 Repro for issue #2529 May 15, 2021
@crusso crusso linked an issue May 15, 2021 that may be closed by this pull request
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

@nomeata PTAL moved the repro to test/mo-idl, it's proper home

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

So the bug is that the Stamp map, used to distinguish instantiation of the same constructor for monomorphisation, was being shared between constructors with the same name but different stamp (and possibly completely different defs). I've now added another outer level of indexing (__k_n) using k to distinguish cons with the same name but different stamp. I suppress k and n when either is 1.

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

In another branch, I'll experiment with normalizing the type to avoid all these weird indirections.

@crusso crusso changed the title Repro for issue #2529 Repro and Fix for issue #2529 May 17, 2021
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks for fix!

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

Yeah, it's an arbitrary choice, 0 is probably better.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

Thanks for fix!

Which raises another question, why do we number form 1, not 0, so the first renaming is always _2 (not _1)? Was there a rationale for this?

Yeah, it's an arbitrary choice, 0 is probably better.

Ok, I'll renumber and also normalize defs.

@crusso
Copy link
Contributor Author

crusso commented May 17, 2021

@chenyan-dfinity PTAL

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

LGTM

@crusso crusso linked an issue May 17, 2021 that may be closed by this pull request
@crusso crusso merged commit 3c6577a into master May 17, 2021
@crusso crusso deleted the claudio/issue-2529 branch May 17, 2021 20:41
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.

Type renamings yield convoluted idl [Bug] Type renaming(s) incorrectly rejected as cyclic
4 participants