Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch - #18881

Merged
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message
Apr 6, 2025
Merged

Fix Incompatible return value type error message for long tuple with Union and non-Union type mismatch#18881
hauntsaninja merged 6 commits into
python:masterfrom
Luunynliny:fix/incompatible_tuple_error_message

Conversation

@Luunynliny

Copy link
Copy Markdown
Contributor

With the following setup :

classA:
a: str|Noneb: str|Nonec: str|Noned: str|Nonee: str|Nonef: str|Noneg: str|Noneh: str|Nonei: str|Nonej: str|Nonek: str|Nonel: str|None@propertydefx(
self,
) ->tuple[
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
str,
]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

mypy outputs this error (you can visualize it in this playground):

mypy_error.py:33: error: Incompatible return value type (0 tuple items are incompatible) [return-value]
Found 1 errors in 1 file (checked 1 source file)

when the expected error should be :

mypy_error.py:33: error: Incompatible return value type (12 tuple items are incompatible; 9 items are omitted) [return-value]
mypy_error.py:33: note: Expression tuple item 0 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 1 has type"str | None";"str" expected; mypy_error.py:33: note: Expression tuple item 2 has type"str | None";"str" expected; Found 1 error in 1 file (checked 1 source file)

Found it is due to the generate_incompatible_tuple_error function from the MessageBuilder class, not checking Union and non-Union mismatch type.

So, I added another condition to increment the error count by ensuring that both type are not the same, instead of just verifying if they are subtype related.

not is_subtype(str | None, str) --> False --> Does not increment error count
not is_same_type(str | None, str) --> True --> Increment error count

I also wrote a new unit test for this case in check-classes.test in the Propertysection.
As it is my first contribution, I'm not sure regarding the location nor the naming of this test, so feel free to suggest a better approach.

@github-actions

This comment has been minimized.

Comment threadmypy/messages.py Outdated
Comment threadtest-data/unit/check-classes.test Outdated
l: Union[str, None]

@property
def x(self) -> Tuple[str, str, str, str, str, str, str, str, str, str, str, str]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a testcase with the opposite direction (annotated with tuple of unions, returns tuple of props)

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@sterliakov Just to make sure I understand, you would like me to test this configuration ?
Or something else ?

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], Union[str, None], Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

@sterliakovsterliakovApr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd love to see a test where the expected return type is wider than the actual return type, but the changed method is still triggered. In your example all items are assignable, so that method won't fire. Something like

classA:
a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: str@propertydefx(self) ->Tuple[Union[str, int], Union[str, float], int, Union[str, None], Union[str, None], Union[str, None], str, str, str, str, str, str]:
return (
self.a,
self.b,
self.c,
self.d,
self.e,
self.f,
self.g,
self.h,
self.i,
self.j,
self.k,
self.l,
)

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is). And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

a: strb: strc: strd: stre: strf: strg: strh: stri: strj: strk: strl: strx: Tuple[
Union[str, int], Union[str, float], int, Union[str, None], Union[str, None],
Union[str, None], str, str, str, str, str, str,
] = (
a, b, c, d, e,
f, g, h, i, j,
k, l,
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a nit: please move this to check-tuples.test as the long tuple is the problem here, not the surrounding class.

@LuunynlinyLuunynlinyApr 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the clarification and the provided example.

I adjusted the third union item to not be assignable, so that the comparator should be triggered (please check that it actually is).

It indeed triggers to relevant error. I also a Union to modified code works as expected.

And why is x a property, won't a simpler test without class do (and same question about the test you already added)?

Beside beging the configuration where I first encountered the error message issue, your suggestion triggers an Incompatible types in assignment (which works correctly) instead of a Incompatible return value type.

@github-actions

This comment has been minimized.

@sterliakovsterliakov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Left a couple of style/scope comments on the testcases, the fix itself looks reasonable. I don't have write rights here, so you'll have to wait for someone else to approve&merge.

Comment threadtest-data/unit/check-classes.test Outdated
Comment threadtest-data/unit/check-classes.test Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fix! I agree with sterliakov's nit that the test belongs better in check-tuples.test (but tests themselves are fine as is, just should be moved to different file)

@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja
hauntsaninja merged commit e9fa89b into python:masterApr 6, 2025
@hauntsaninja

Copy link
Copy Markdown
Collaborator

Thank you!!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Luunynliny@hauntsaninja@brianschubert@sterliakov