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

Lowercase type reference directives when determining to reuse program structure (just like when we create new program) #26944

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Sep 6, 2018

Without this change when program contains a file that references UpperCase package using typeReferenceDirective, trying to reuse the old program we will resolve type reference directive as "UpperCasePackage" and that can succeed, thus us identifying that program structure to reuse is only module safe (and cant be used completely because typereference directive from old prgram to "uppercasepackage" and new "UpperCasePackage" wont match). Now when creating the actual new program we will resolve type reference directive with "UpperCasePacakage" while doing so we will release the "UpperCasePackage" from ResolutionCache and thus having deferred watch for "UpperCasePackage" that is now refCount to 0 and failing in assert later while finishing the resolution cache.

… structure (just like when we create new program)
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

LGTM, assuming you reviewed other usages of module names to confirm they didn't need the same transformation.

@sheetalkamat
Copy link
Member Author

sheetalkamat commented Sep 6, 2018

@amcasey There is still one more usage for auto type reference directives, but it is always done in same place so doesn't break the resolution cache by removing the created resolution in that pass itself. But question still remains if we should be lower casing the name there. but that would be independent of this PR. Also those are the package names read using readDirectory so I think having them as is better and if we change that it would be a breaking change.

@sheetalkamat sheetalkamat merged commit 6fb0f68 into master Sep 6, 2018
@sheetalkamat sheetalkamat deleted the casingOfTypeReferenceDirectives branch September 6, 2018 21:13
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.

2 participants