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

Prevent mutation of double-cached cells #429

Merged
merged 2 commits into from Jun 2, 2022

Conversation

tvt173
Copy link
Collaborator

@tvt173 tvt173 commented Jun 2, 2022

Hi @joamatab,

This patch fixes some difficult-to-find errors having to do with mutability (you'll see it even fixes one of the existing tests, for the grating_coupler_tree!). Essentially, before adding a component to the CACHE, we first check if the component has already been added or not, under a different name. If it has, it copies the component before adding it again to the CACHE. This prevents errors that could arise in a variety of cases, for example

  • cells which have already been instantiated, but are re-called with an explicit name supplied
  • cells which are returned as-is inside a new @cell-decorated function, without being first copied
  • different from_yaml cells, which have identical content

Furthermore I remove calls to unlock() within cell() and import_gds(), which I think makes everything a bit safer! Overall, I hope this MR takes a bit of a mental load off the developer, not to have to remember these patterns which are potentially dangerous. Instead, a MutabilityError should be thrown reliably in those cases.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jun 2, 2022

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 0.25%.

Quality metrics Before After Change
Complexity 24.29 😞 24.92 😞 0.63 👎
Method Length 105.35 🙂 106.45 🙂 1.10 👎
Working memory 17.27 ⛔ 17.25 ⛔ -0.02 👍
Quality 34.75% 😞 34.50% 😞 -0.25% 👎
Other metrics Before After Change
Lines 624 632 8
Changed files Quality Before Quality After Quality Change
gdsfactory/cell.py 42.17% 😞 41.51% 😞 -0.66% 👎
gdsfactory/read/import_gds.py 12.51% ⛔ 12.68% ⛔ 0.17% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
gdsfactory/read/import_gds.py import_gds 40 ⛔ 474 ⛔ 25 ⛔ 8.10% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
gdsfactory/cell.py cell_without_validator 42 ⛔ 521 ⛔ 22 ⛔ 8.41% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
gdsfactory/cell.py cell_without_validator._cell 27 😞 515 ⛔ 22 ⛔ 14.48% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
gdsfactory/cell.py test_names 0 ⭐ 139 😞 6 ⭐ 73.21% 🙂 Try splitting into smaller methods

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.8% 1.8% Duplication

@joamatab
Copy link
Contributor

joamatab commented Jun 2, 2022

Awesome! thank you Troy! releasing 5.8.4 with your improvements

@joamatab joamatab merged commit 18e4c60 into gdsfactory:master Jun 2, 2022
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.

None yet

2 participants