Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Name mangling for imported internal declarations (after symbol resolution)#370

Merged
bamarsha merged 34 commits intofeatures/access-modifiersfrom
marshallsa/rename-later
Mar 16, 2020
Merged

Name mangling for imported internal declarations (after symbol resolution)#370
bamarsha merged 34 commits intofeatures/access-modifiersfrom
marshallsa/rename-later

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Mar 13, 2020

This is an alternative to PR #360, part of issue #259. Instead of name mangling as soon as references are loaded (before symbol resolution), name mangling is run just before the syntax tree is built in QsCompilation.Build. The benefit is that it is easier to show specific error messages for inaccessible symbols, instead of an "unknown symbol" message.

Because this happens after symbol resolution, the symbol table needs to be updated to accommodate multiple declarations for a single callable or type if some of them belong to a project/package reference. Thus, the logic for resolving symbols in the symbol table has been refactored so that it is more flexible.

TODO in this PR:

  • Allow shadowing imported internal callables
  • Allow shadowing imported internal types
  • Run name mangling in QsCompilation.Build
  • Add tests for name mangling
  • Add tests for redeclarations in more than one reference
  • Decide what to do about some assertions that were disabled (marked with TODO comments)

TODO in the next PR:

  • Allow redeclarations of internal items in more than one reference
  • Group internal specializations with the same name in different references correctly

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

It looks like good progress. I assume the only failing tests are the ones you added for multiple references with the same internal declaration? If so, then I'd suggest to comment those out and make a separate PR that reenables those tests and addresses the remaining two boxes on your todo list.

@bamarsha bamarsha marked this pull request as ready for review March 14, 2020 20:36
Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

Looks good. One minor thing related to the test project setup.

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

Somewhere in this PR you have lost the range information for udts in argument tuples. There is a failing code gen test in the runtime repo.

@bettinaheim
Copy link
Contributor

bettinaheim commented Mar 16, 2020

@MarshallSA Found the issue: The changes here seem to have gotten lost. I'll make a PR to this branch shortly.

@bamarsha bamarsha merged commit 6861418 into features/access-modifiers Mar 16, 2020
@bamarsha bamarsha deleted the marshallsa/rename-later branch March 16, 2020 22:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants