Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston
, '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

Annotate System.Linq.Expressions with RequiresDynamicCode - #90456

Merged
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e
Aug 15, 2023
Merged

Annotate System.Linq.Expressions with RequiresDynamicCode#90456
agocke merged 11 commits into
dotnet:mainfrom
agocke:annotate-s-l-e

Conversation

@agocke

Copy link
Copy Markdown
Member

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

All this ended up with an RUC on Expression.Compile due to new arrays.
I could potentially silence this warning with a feature flag, but it is a
real risk, and one that users could maybe work around if alterted to the problem.
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

All this ended up with an RDC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk in AOT, and one that users could maybe work around if alerted to the problem.

Author:agocke
Assignees:agocke
Labels:

area-System.Linq.Expressions

Milestone:-

/// Produces a delegate that represents the lambda expression.
/// </summary>
/// <returns>A delegate containing the compiled version of the lambda.</returns>
[RequiresDynamicCode(Expression.NewArrayRequiresDynamicCode)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message is a bit weird (IMO). Anytime someone tries to compile an expression, they will get the warning

"Creating arrays at runtime requires dynamic code generation. This warning can be suppressed if there are no new array nodes in the expression tree."

But won't they already get the warning from the call that actually creates new array nodes in the expression tree? Therefore, we don't need to also warn them at Compile().

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, hadn't thought of that. The problem is that something like Expression<Func<int[]>> e = () => new[] { 1 }; won't produce an analyzer warning since we don't see the call site, but maybe that can be OK for now and we can treat it as an analyzer improvement to fix later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, wouldn't it be fine? There's code there for a new int[].

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, maybe. Replace int with your favorite custom struct. That likely wouldn't work.

agockeand others added 2 commits August 13, 2023 14:04
@agocke

Copy link
Copy Markdown
MemberAuthor

@eerhardt Took your suggestion. There's now a suppression at the NewArrayExpression conversion routine.

@stephentoub
stephentoub requested a review from cstonAugust 14, 2023 21:24
Comment threadsrc/libraries/System.Linq.Expressions/src/Resources/Strings.resx Outdated
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
Comment threadsrc/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs Outdated
@agocke
agocke merged commit 067a357 into dotnet:mainAug 15, 2023
@agocke
agocke deleted the annotate-s-l-e branch August 15, 2023 02:45
@eerhardt

Copy link
Copy Markdown
Member

@agocke - looks like this just missed the 8.0 cutoff. Do we want to backport this into .NET 8?

@agocke

Copy link
Copy Markdown
MemberAuthor

I think it would be a good idea, given that people are hitting this in the wild

@agocke

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-rc1

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5870002616

@ghostghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@agocke@eerhardt@stephentoub@cston